feat(starrocks): add catalog support for StarRocks database connections#37026
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #1da233Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Nitpicks 🔍
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| mock_row_1 = mocker.MagicMock() | ||
| mock_row_1.keys.return_value = ["Catalog", "Type", "Comment"] | ||
| mock_row_1.__getitem__ = lambda self, key: "default_catalog" if key == "Catalog" else None | ||
|
|
||
| mock_row_2 = mocker.MagicMock() | ||
| mock_row_2.keys.return_value = ["Catalog", "Type", "Comment"] | ||
| mock_row_2.__getitem__ = lambda self, key: "hive" if key == "Catalog" else None | ||
|
|
||
| mock_row_3 = mocker.MagicMock() | ||
| mock_row_3.keys.return_value = ["Catalog", "Type", "Comment"] | ||
| mock_row_3.__getitem__ = lambda self, key: "iceberg" if key == "Catalog" else None |
There was a problem hiding this comment.
Suggestion: The mocks for catalog rows override __getitem__ on MagicMock instances, but special methods are looked up on the class, so row["Catalog"] still returns MagicMock objects instead of the expected strings, causing get_catalog_names to return a set of mocks instead of {"default_catalog", "hive", "iceberg"} and making the test assertions fail. [logic error]
Severity Level: Minor
| mock_row_1 = mocker.MagicMock() | |
| mock_row_1.keys.return_value = ["Catalog", "Type", "Comment"] | |
| mock_row_1.__getitem__ = lambda self, key: "default_catalog" if key == "Catalog" else None | |
| mock_row_2 = mocker.MagicMock() | |
| mock_row_2.keys.return_value = ["Catalog", "Type", "Comment"] | |
| mock_row_2.__getitem__ = lambda self, key: "hive" if key == "Catalog" else None | |
| mock_row_3 = mocker.MagicMock() | |
| mock_row_3.keys.return_value = ["Catalog", "Type", "Comment"] | |
| mock_row_3.__getitem__ = lambda self, key: "iceberg" if key == "Catalog" else None | |
| mock_row_1 = {"Catalog": "default_catalog", "Type": None, "Comment": None} | |
| mock_row_2 = {"Catalog": "hive", "Type": None, "Comment": None} | |
| mock_row_3 = {"Catalog": "iceberg", "Type": None, "Comment": None} |
Why it matters? ⭐
The claim is correct: assigning getitem on MagicMock instances does not affect Python's special-method lookup which resolves special methods on the object's class. As written the test may yield MagicMock values when code does row["Catalog"] (or similar), causing the assertion to fail or be flaky. Replacing the MagicMock rows with plain dicts (or configuring the mock's class-level getitem) produces real strings and makes the test deterministic. This is a real logic fix for the unit test rather than a cosmetic change.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/db_engine_specs/test_starrocks.py
**Line:** 225:235
**Comment:**
*Logic Error: The mocks for catalog rows override `__getitem__` on `MagicMock` instances, but special methods are looked up on the class, so `row["Catalog"]` still returns MagicMock objects instead of the expected strings, causing `get_catalog_names` to return a set of mocks instead of `{"default_catalog", "hive", "iceberg"}` and making the test assertions fail.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
CodeAnt AI finished reviewing your PR. |
74996ff to
738d37e
Compare
738d37e to
19a83c2
Compare
Code Review Agent Run #a8e334Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
villebro
left a comment
There was a problem hiding this comment.
Lots of great improvements to this spec, thanks for this!
- theming.mdx: document ECharts array property overrides (PR #37965) — array values like color palettes are fully supported and replaced entirely (not merged); adds Array Property Overrides section with color palette example - configuring-superset.mdx: document PKCE support for database OAuth2 (PR #37067) — add PKCE section under Custom OAuth2 with code_challenge_method config and when to use it - cache.mdx: document ETag support for thumbnail/screenshot endpoints (PR #37663) — conditional GET with If-None-Match to avoid downloading unchanged images - exploring-data.mdx: document SQL Lab UX improvements (PRs #37298, #37694, #37756) — treeview table browser, Ctrl+F find widget, resizable panels; also adds time range natural language expressions section (PR #37098) - creating-your-first-dashboard.mdx: document Table chart features — boolean and categorical conditional formatting (PRs #36338, #37756), gradient toggle (PR #36280), HTML cell rendering with security note (PR #37685), column header tooltips from dataset descriptions (PR #37179), Display Controls modal in dashboard view (PR #36062) - databases.json: update StarRocks supports_catalog and supports_dynamic_catalog to true — the engine spec (PR #37026) already implemented full catalog support with get_catalog_names(), get_default_catalog(), and SHOW CATALOGS; the committed JSON was stale and did not reflect this Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
SUMMARY
Adds comprehensive catalog support to the StarRocks database engine specification, enabling users to browse and query data across multiple catalogs (e.g., iceberg, hive, default_catalog).
Key changes:
supports_catalog,supports_dynamic_catalog,supports_dynamic_schema)get_catalog_names()to retrieve available catalogs viaSHOW CATALOGSget_default_catalog()to identify the default catalog from the URIadjust_engine_params()to handlecatalog.schemaURI formatget_schema_names()to query schemas usingSHOW DATABASEScatalog.dbis optionalConnection string format:
starrocks://host:port/catalog.schema- connects to specific schema in catalogstarrocks://host:port/catalog.- sets catalog context for browsing schemasstarrocks://host:port- no catalog or schema specifiedBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - Backend-only change
TESTING INSTRUCTIONS
pytest tests/unit_tests/db_engine_specs/test_starrocks.py -vADDITIONAL INFORMATION