-
Notifications
You must be signed in to change notification settings - Fork 150
fix(ibis): handle JSON result for MySQL #1377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughIntroduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bc6d8a8 to
8fba3c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ibis-server/app/model/connector.py (1)
323-353: JSON casting behavior for MySQL looks correct; consider delegating to base handlerThe override mirrors the base
IbisConnectorbehavior forDecimalandUUIDand addsdt.JSONcasting to"string", which should fix pyarrow incompatibility for MySQL JSON columns while preserving existing numeric/UUID handling.Two small follow‑ups to consider:
- You could call
super()._handle_pyarrow_unsupported_typefirst and then only loop fordt.JSONto avoid duplicating the Decimal/UUID logic and keep MySQL aligned with any future changes to the base handler.- It may be worth verifying (via a quick schema inspection in tests) that MySQL array/struct values that come back as JSON are indeed typed as
dt.JSONby ibis; if some surface as other types (e.g.,dt.Array/dt.Struct), they would currently bypass this casting.Example refactor sketch:
def _handle_pyarrow_unsupported_type(self, ibis_table: Table, **kwargs) -> Table: result_table = super()._handle_pyarrow_unsupported_type(ibis_table, **kwargs) for name, dtype in result_table.schema().items(): if isinstance(dtype, dt.JSON): result_table = self._cast_json_columns(result_table=result_table, col_name=name) return result_tableibis-server/tests/routers/v3/connector/mysql/test_query.py (2)
9-41: Manifest definition is clear; fixture can be synchronousThe manifest mirrors the
json_testschema and MySQL data source cleanly, and the base64+orjsonencoding matches existing patterns. Sincemanifest_strdoesn’t perform any async I/O, you can simplify it to a regular synchronous fixture:@pytest.fixture(scope="module") def manifest_str(): return base64.b64encode(orjson.dumps(manifest)).decode("utf-8")This avoids depending on async‑fixture behavior for something that’s purely in‑memory.
44-67: JSON query test correctly validates cast‑to‑string; watch out for key‑order brittlenessThe test does a good job of:
- Disabling fallback so it hits the ibis/MySqlConnector path.
- Verifying both JSON columns are surfaced as
"string"dtypes.- Asserting representative row values from
json_test.The strict equality check on the full JSON string (including key order) for
object_colcould be a bit brittle if MySQL/ibis/serialization ever changes ordering; parsing the value as JSON and asserting on the resulting dict would make the test more robust while still confirming that JSON is returned as a string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ibis-server/app/model/connector.py(2 hunks)ibis-server/tests/routers/v3/connector/mysql/conftest.py(2 hunks)ibis-server/tests/routers/v3/connector/mysql/test_functions.py(1 hunks)ibis-server/tests/routers/v3/connector/mysql/test_query.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ibis-server/app/model/connector.py (1)
ibis-server/app/model/data_source.py (1)
DataSource(62-221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci
- GitHub Check: ci
🔇 Additional comments (4)
ibis-server/tests/routers/v3/connector/mysql/conftest.py (2)
4-52: MySQL container + JSON seed setup looks solidUsing
MySqlContainerplus a SQLAlchemy engine to create and populatejson_testwith JSON columns is clear and scoped to the session; the explicitcommit()ensures data is ready for the JSON tests. This is a good place to centralize MySQL JSON test data.
57-75: Centralizing remote function list configuration in mysql conftestDefining
function_list_path/white_function_list_pathhere and wiring them via autouse fixtures keeps MySQL tests consistent and avoids duplication. The fixtures correctly set and then reset the config on each test, so global state shouldn’t leak across tests.ibis-server/app/model/connector.py (1)
101-102: MySQL data source correctly routed to MySqlConnectorAdding the explicit
DataSource.mysqlbranch ensures MySQL goes through the new connector and its custom type handling instead of the genericIbisConnector, which is the right integration point.ibis-server/tests/routers/v3/connector/mysql/test_functions.py (1)
7-12: Reusing function list paths from mysql conftest is a good consolidationImporting
base_url,function_list_path, andwhite_function_list_pathfrom the shared mysqlconftestkeeps this test aligned with the centralized configuration and avoids hard‑coding paths in multiple places. The explicit set/reset logic intest_function_liststill makes the intended behavior clear.
MySQL uses JSON to present
ARRAYandSTRUCTvalues. However, ibis doesn't handle JSON type correctly. To show the result, we cast the JSON value to text for display purposes.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.