fix(metadb): apply SUPERSET_META_DB_LIMIT after join instead of per-table (#36304) - #42598
fix(metadb): apply SUPERSET_META_DB_LIMIT after join instead of per-table (#36304)#42598rusackas wants to merge 4 commits into
Conversation
Closes #36304 SUPERSET_META_DB_LIMIT is applied to each underlying table independently, before the in-memory join runs. A row that has a genuine match on the other side of the join but falls past the per-table limit is silently dropped from the join result, with no error or truncation warning. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #6822e5Actionable Suggestions - 0Review 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 AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| @with_config( | ||
| { | ||
| "DB_SQLA_URI_VALIDATOR": None, | ||
| "SUPERSET_META_DB_LIMIT": 2, | ||
| "DATABASE_OAUTH2_CLIENTS": {}, | ||
| "SQLALCHEMY_CUSTOM_PASSWORD_STORE": None, | ||
| } | ||
| ) | ||
| @with_feature_flags(ENABLE_SUPERSET_META_DB=True) |
There was a problem hiding this comment.
Suggestion: This test uses with_config, whose wrapper restores configuration only after the wrapped function returns. If this test fails or raises during engine creation or query execution, SUPERSET_META_DB_LIMIT remains set to 2 in the shared Flask application and can alter subsequent tests, causing order-dependent failures. The configuration helper needs exception-safe restoration, or this test needs equivalent cleanup. [stale reference]
Severity Level: Major ⚠️
- ❌ Failed tests can contaminate subsequent test configuration.
- ⚠️ Later metadata queries may unexpectedly limit rows.
- ⚠️ Test outcomes can become order-dependent.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/extensions/test_sqlalchemy.py
**Line:** 276:284
**Comment:**
*Stale Reference: This test uses `with_config`, whose wrapper restores configuration only after the wrapped function returns. If this test fails or raises during engine creation or query execution, `SUPERSET_META_DB_LIMIT` remains set to `2` in the shared Flask application and can alter subsequent tests, causing order-dependent failures. The configuration helper needs exception-safe restoration, or this test needs equivalent cleanup.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Already fixed on the branch, swapped @with_config for monkeypatch so a failed assertion here still restores SUPERSET_META_DB_LIMIT for later tests instead of leaking it.
| engine = create_engine("superset://", future=True) | ||
| except Exception as e: | ||
| # Skip test if superset:// dialect can't be loaded (common in Docker) | ||
| pytest.skip(f"Superset dialect not available: {e}") |
There was a problem hiding this comment.
Suggestion: The handler skips the test for every exception raised while creating the engine, not just a missing dialect. Configuration errors, broken dependencies, and other genuine regressions will therefore be reported as a skipped test instead of failing CI, masking whether this regression is covered. [possible bug]
Severity Level: Major ⚠️
- ❌ Engine regressions can be reported as skipped.
- ⚠️ The new join regression may receive no coverage.
- ⚠️ CI can remain green despite broken initialization.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/extensions/test_sqlalchemy.py
**Line:** 309:312
**Comment:**
*Possible Bug: The handler skips the test for every exception raised while creating the engine, not just a missing dialect. Configuration errors, broken dependencies, and other genuine regressions will therefore be reported as a skipped test instead of failing CI, masking whether this regression is covered.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
That broad except is the same skip pattern every other test in this file uses (test_superset, test_superset_limit, test_dml, etc), just reused for the new one. Not going to special-case it here, would rather keep it consistent.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
In SQLAlchemy extension tests, allow broad exception handling around engine creation when it follows the file's established skip pattern; do not require special-casing individual exceptions for consistency.
Applied to:
tests/unit_tests/extensions/**
💡 To manage or update this instruction, visit: CodeAnt AI Settings
| results = conn.execute( | ||
| text(""" | ||
| SELECT t1.b, t2.b | ||
| FROM "database1.table1_large" AS t1 | ||
| JOIN "database2.table2_late_match" AS t2 | ||
| ON t1.a = t2.a | ||
| """) | ||
| ) |
There was a problem hiding this comment.
Suggestion: The test applies LIMIT 2 without an ORDER BY, so the database is free to return any two rows. If row a=3 is included instead of a=1 or a=2, the join can return the expected row even while the per-table truncation bug remains, making this regression test nondeterministic and potentially ineffective. Add an explicit ordering to make the truncated rows deterministic. [logic error]
Severity Level: Major ⚠️
- ❌ Regression test can pass while truncation remains.
- ⚠️ Join coverage depends on backend row-order behavior.
- ⚠️ CI results may vary across database configurations.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/extensions/test_sqlalchemy.py
**Line:** 315:322
**Comment:**
*Logic Error: The test applies `LIMIT 2` without an `ORDER BY`, so the database is free to return any two rows. If row `a=3` is included instead of `a=1` or `a=2`, the join can return the expected row even while the per-table truncation bug remains, making this regression test nondeterministic and potentially ineffective. Add an explicit ordering to make the truncated rows deterministic.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
With the actual adapter fix landed, join queries skip the per-table limit entirely now, so there's no truncation happening here for row order to matter. Should be moot at this point.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42598 +/- ##
==========================================
+ Coverage 65.33% 65.38% +0.04%
==========================================
Files 2803 2810 +7
Lines 158490 159212 +722
Branches 36178 36287 +109
==========================================
+ Hits 103557 104099 +542
- Misses 52922 53070 +148
- Partials 2011 2043 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The new test_superset_joins_with_limit_drops_matches is expected to fail (it pins down issue #36304, which isn't fixed yet). It used the @with_config decorator, which only restores overridden config keys after the wrapped test returns normally -- an assertion failure skips the restore. That left SUPERSET_META_DB_LIMIT=2 leaked into the next test in the file, test_dml, causing its SELECT to be truncated and fail collaterally. Switch to monkeypatch.setitem(current_app.config, ...), which is undone unconditionally regardless of test outcome, matching the existing pattern used elsewhere in the unit test suite. Co-Authored-By: Claude <noreply@anthropic.com>
| try: | ||
| engine = create_engine("superset://", future=True) | ||
| except Exception as e: | ||
| # Skip test if superset:// dialect can't be loaded (common in Docker) | ||
| pytest.skip(f"Superset dialect not available: {e}") |
There was a problem hiding this comment.
Suggestion: The broad Exception handler skips the test for every engine-construction failure, including genuine regressions in dialect registration, feature-flag setup, or configuration. This can turn a broken implementation into a silently skipped test instead of a CI failure; restrict the skip to the specific missing-dialect/import error or allow unexpected exceptions to propagate. [possible bug]
Severity Level: Critical 🚨
- ❌ CI can silently skip the regression test.
- ⚠️ Dialect initialization regressions remain undetected.
- ⚠️ Coverage for issue #36304 becomes environment-dependent.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/extensions/test_sqlalchemy.py
**Line:** 313:317
**Comment:**
*Possible Bug: The broad `Exception` handler skips the test for every engine-construction failure, including genuine regressions in dialect registration, feature-flag setup, or configuration. This can turn a broken implementation into a silently skipped test instead of a CI failure; restrict the skip to the specific missing-dialect/import error or allow unexpected exceptions to propagate.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Same as the other thread on this line, that's copied from the skip-if-dialect-unavailable pattern already used by every test in the file. Leaving it as-is for consistency rather than fixing it in just the new test.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not flag the broad exception handling used to skip unavailable-dialect tests when following the established pattern in this file.
Applied to:
tests/unit_tests/extensions/test_sqlalchemy.py
💡 To manage or update this instruction, visit: CodeAnt AI Settings
Code Review Agent Run #4f8126Actionable Suggestions - 0Review 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 AI Code Review powered by |
| # table1_large (a=3, b=30), but SUPERSET_META_DB_LIMIT=2 truncates | ||
| # table1_large to its first two rows (a=1, a=2) before the join | ||
| # runs, so the join comes back empty instead of finding the match. | ||
| assert list(results) == [(30, "thirty")] |
There was a problem hiding this comment.
This assertion is the sole failure in the required unit-test job, so this test-only change cannot merge or close #36304 while the adapter is unchanged. Could the implementation fix land with this regression test?
There was a problem hiding this comment.
Landed the adapter fix on top. SupersetAPSWDialect now tracks whether a statement touches more than one table and skips the per-table cap when it does, so the regression test should be green.
…les (#36304) Shillelagh's adapter interface calls SupersetShillelaghAdapter.get_data once per underlying table, independently of any other table referenced by the same statement. get_data had no way to tell whether it was being asked for a standalone table or for one side of a join, so it applied SUPERSET_META_DB_LIMIT to every table unconditionally. For a join, this silently truncated a joined table before the in-memory join ran, dropping rows that had a genuine match on the other side, with no error. Fix: SupersetAPSWDialect now overrides do_execute/do_execute_no_params/ do_executemany to record (via a contextvar, for the duration of that one statement) whether the SQL text contains a JOIN. get_data consults that flag and only falls back to the app-wide default limit when the statement doesn't join tables, where truncating a table's row read can't hide otherwise-valid matches. The default limit still protects plain single-table reads (test_superset_limit) and real SQL-level LIMIT clauses pushed down by Shillelagh/SQLite continue to be honored/capped as before. Co-Authored-By: Claude <noreply@anthropic.com>
rusackas
changed the title
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| # standalone table or for one side of a join. `SupersetAPSWDialect.do_execute*` | ||
| # populates `_executing_join_query` for the duration of a statement so that | ||
| # `get_data` can tell the two cases apart (see `get_data` for why this matters). | ||
| _JOIN_KEYWORD_RE = re.compile(r"\bJOIN\b", re.IGNORECASE) |
There was a problem hiding this comment.
Suggestion: Detecting joins solely through the literal JOIN keyword misses valid SQLite joins written as comma-separated tables, such as FROM table1, table2 WHERE table1.id = table2.id. Both virtual tables are still fetched independently, but _executing_join_query remains false and the configured cap can truncate one side before the join, dropping valid results. Detect multi-table query shapes using SQL parsing or a reliable SQLite execution signal instead of only matching JOIN. [logic error]
Severity Level: Major ⚠️
- ❌ Comma-style cross-database joins can return incomplete results.
- ⚠️ Valid SQLite join syntax bypasses the join-specific limit handling.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/extensions/metadb.py
**Line:** 83:83
**Comment:**
*Logic Error: Detecting joins solely through the literal `JOIN` keyword misses valid SQLite joins written as comma-separated tables, such as `FROM table1, table2 WHERE table1.id = table2.id`. Both virtual tables are still fetched independently, but `_executing_join_query` remains false and the configured cap can truncate one side before the join, dropping valid results. Detect multi-table query shapes using SQL parsing or a reliable SQLite execution signal instead of only matching `JOIN`.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Good catch, a comma join references two tables without the literal JOIN keyword. Swapped the detection to count quoted db.table identifiers instead of matching JOIN, covers both syntaxes now, and added a regression test for the comma-join case.
| with self._flag_join_query(statement): | ||
| super().do_execute(cursor, statement, parameters, context) |
There was a problem hiding this comment.
Suggestion: The join flag is reset when super().do_execute returns, but APSW/SQLite can defer virtual-table row retrieval until the returned cursor is iterated. In that case get_data runs after this context manager exits, sees _executing_join_query as false, and still applies the per-table limit, so the regression remains for normal result consumption. Keep the flag active through cursor iteration or use an execution-scoped mechanism that covers virtual-table callbacks. [api mismatch]
Severity Level: Critical 🚨
- ❌ Cross-database joins can still return incomplete results.
- ⚠️ The regression test may fail despite the intended execution wrapper.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/extensions/metadb.py
**Line:** 144:145
**Comment:**
*Api Mismatch: The join flag is reset when `super().do_execute` returns, but APSW/SQLite can defer virtual-table row retrieval until the returned cursor is iterated. In that case `get_data` runs after this context manager exits, sees `_executing_join_query` as false, and still applies the per-table limit, so the regression remains for normal result consumption. Keep the flag active through cursor iteration or use an execution-scoped mechanism that covers virtual-table callbacks.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Checked this empirically with a debug print inside get_data during the join test, and it's called synchronously inside do_execute for this APSW/shillelagh setup, the flag is still set when it runs. Not seeing this materialize in practice, but flag it again if you spot a case where it does.
There was a problem hiding this comment.
I traced this past the first SQLite step, and the deferral does materialize once the result has multiple rows: Shillelagh converts the APSW cursor through a lazy generator, so later inner-table rescans invoke get_data during result iteration after do_execute has reset this flag. That reapplies the per-table cap after the first outer row and can still drop later matches; could the flag lifetime cover cursor iteration, with a regression test that returns multiple join rows?
There was a problem hiding this comment.
The concern is valid. do_execute* only covers statement execution, while Shillelagh may defer virtual-table reads until the APSW cursor is consumed. Consequently, the ContextVar can be reset before later get_data calls, causing the limit to be reapplied during inner-table rescans.
The flag should remain active for the cursor’s lifetime rather than only for super().do_execute*. A concise fix is to wrap the returned cursor/fetch iteration and reset the context variable only when the cursor is exhausted or closed. The wrapper must also preserve the existing token-reset behavior on execution errors.
The regression coverage should include multiple joined rows, for example:
SELECT t1.b, t2.b
FROM "database1.table1_large" AS t1
JOIN "database2.table2_late_match" AS t2
ON t1.a = t2.awith table2_late_match containing matches for both a = 2 and a = 3, while SUPERSET_META_DB_LIMIT = 2. The expected result must contain both rows; otherwise the test would not detect a limit being reapplied during later cursor iteration.
Thus, this comment should be addressed: the current execution-scoped flag is insufficient for lazy result consumption.
Code Review Agent Run #2cc55bActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 AI Code Review powered by |
…check The JOIN-keyword regex used to decide whether the per-table row limit is safe to apply missed implicit comma joins (`FROM a, b WHERE ...`), which reference multiple superset:// virtual tables just like an explicit JOIN but without the literal keyword. Detect multi-table statements instead by counting quoted `database.table` identifiers, which covers both syntaxes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #208e5fActionable Suggestions - 0Review 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 AI Code Review powered by |
| # `SupersetAPSWDialect.do_execute*` populates `_executing_multi_table_query` | ||
| # for the duration of a statement so that `get_data` can tell the two cases | ||
| # apart (see `get_data` for why this matters). | ||
| _TABLE_REF_RE = re.compile(r'"[^"]*\.[^"]*"') |
There was a problem hiding this comment.
This regex also matches dotted quoted column aliases, not just table references: for example, SELECT COUNT(id) AS "metric.value" FROM "database1.table1" produces two matches and silently disables SUPERSET_META_DB_LIMIT for a single-table read. Could this detect the statement’s actual virtual tables (the existing SQL parser exposes them) rather than counting every dotted quoted token?
| # error (see #36304). Only fall back to the default for statements | ||
| # that reference a single table, where truncating it can't hide | ||
| # otherwise-valid matches. | ||
| if app_limit is not None and not _executing_multi_table_query.get(): |
There was a problem hiding this comment.
For every multi-table statement this leaves limit=None, so each underlying source can be read in full before SQLite performs the join; an outer result limit does not necessarily bound those virtual-table scans. On two large remote tables this turns the old 1,000-row guardrail into an unbounded worker-memory load—should the join path retain a separate configurable scan ceiling or another explicit safeguard?
3 participants
SUMMARY
#36304 reports that a cross-database join via the
ENABLE_SUPERSET_META_DBfeature returns "no data" once aWHERE ... OR(or equivalentIN) filter is added, even though the same query with a single equality filter returns rows. Two bot theories were floated and refuted in the issue thread (single-filter-per-column limitation); the real root cause, confirmed by tracingsuperset/extensions/metadb.py, isSUPERSET_META_DB_LIMIT(default 1000):SupersetSQLiteAdapter.get_dataapplied it to each underlying table independently, before Shillelagh/SQLite runs the in-memory join. If a joined table has more rows than the limit, only the firstSUPERSET_META_DB_LIMITrows were ever read from it — so a row with a genuine match on the other side of the join could be silently truncated away before the join logic even sees it, producing an incomplete or empty result with no error. Docs for this caveat were already added in #41302; this PR pins down the actual behavior with a regression test (test_superset_joins_with_limit_drops_matches) and fixes the underlying bug.THE FIX
Shillelagh's adapter interface calls
SupersetShillelaghAdapter.get_dataonce per underlying table, independently of any other table referenced by the same statement —get_datahas no built-in way to tell whether it's being asked for a standalone table or for one side of a join. It was applyingSUPERSET_META_DB_LIMITunconditionally whenever no real SQL-levelLIMITwas pushed down to that table by Shillelagh/SQLite, which happens for both single-table reads and joined tables (there's no explicitLIMITclause in either case).The fix has
SupersetAPSWDialectoverridedo_execute/do_execute_no_params/do_executemanyto record, via acontextvars.ContextVarscoped to the duration of executing that one statement, whether the SQL text contains aJOIN.SupersetShillelaghAdapter.get_dataconsults that flag: it only falls back to the app-wideSUPERSET_META_DB_LIMITdefault when the statement being executed doesn't join tables, since only then can truncating a table's row read not hide an otherwise-valid match. This:SUPERSET_META_DB_LIMITstill capsSELECT * FROM "db.table"with no explicitLIMIT, covered bytest_superset_limit).LIMITclauses untouched: when Shillelagh/SQLite has already decided it's safe to push an actualLIMITdown to a specific table (which it only ever does when correctness is provable), that value is still capped withmin(limit, app_limit)exactly as before.TESTING INSTRUCTIONS
All 7 tests in the file pass, including the regression test
test_superset_joins_with_limit_drops_matches, which now passes for real (not by weakening the assertion).ADDITIONAL INFORMATION
ENABLE_SUPERSET_META_DB(already required by existing tests in this file)🤖 Generated with Claude Code