[fix](connector) rebuild connector scan properties after column pruning - #66369
Open
morningman wants to merge 2 commits into
Open
[fix](connector) rebuild connector scan properties after column pruning#66369morningman wants to merge 2 commits into
morningman wants to merge 2 commits into
Conversation
A connector never decides which columns to read: it renders whatever list `ConnectorScanRequest.getColumns()` carries. The jdbc connector turns that list verbatim into the remote SELECT list and falls back to `SELECT *` when it is empty, so the whole of column pruning for every plugin-driven external scan rests on `PluginDrivenScanNode.buildColumnHandles()` intersecting the connector's column handles with the scan's tuple slots. That method had no direct coverage, and the failure mode it guards against -- projecting more columns than the query needs -- is a pure performance regression that no result-comparing test can observe. This adds: - PluginDrivenScanNodeColumnPruningTest: drives the real projection and pins that only tuple-slot columns are projected, that the order follows the slot order (the connector renders the list positionally), that column-less and unmatched slots are skipped, and that an empty tuple projects nothing -- the sole input that reaches the connector's `SELECT *` fallback. - an explain assertion in test_mysql_jdbc_catalog for `count(*)`, the one query shape whose projection would otherwise go empty. The engine keeps a single smallest slot instead, so the remote SQL must stay one column wide rather than degrading to a 12-column `SELECT *` just to count rows. The assertion pins the arity of the select list, not which column wins, since that is `getSmallestSlot`'s business and tracks type widths. Test-only change; no production code is touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28722 ms |
Contributor
TPC-DS: Total hot run time: 169798 ms |
Contributor
ClickBench: Total hot run time: 23.89 s |
A plugin-driven scan asks its connector for one property bundle -- the jdbc remote SELECT, per-column dictionaries, file format, path partition keys -- and caches it. That cache is first filled from `init()` (`FileQueryScanNode.initSchemaParams` -> `getPathPartitionKeys`), which runs while `PhysicalPlanTranslator` is still translating this scan, i.e. strictly before the project above it prunes the tuple down to the columns the query actually reads. Everything the connector derives from the projection at that point therefore describes the FULL table schema. Only `convertPredicate()` dropped that cache, and only when there was a conjunct to push down. Queries with a WHERE clause were rebuilt from the pruned tuple by accident; filter-less ones kept the pre-pruning bundle. So `explain select count(*) from <jdbc table>` reported a 12-column remote read of a 12-column table, and every WHERE-less query on any plugin-driven external table reported a full-width read. The scan itself was unaffected -- `getSplits` rebuilds the column handles from the final tuple -- but the reported remote query was wrong, and so is anything else a connector derives from the projection through this bundle (`populateScanLevelParams`, `getFileAttributes`). Drop the cache in `doFinalize()` instead: that is the first point at which the tuple is final. Every filtered query already exercises this rebuild path today, including the second MVCC-snapshot / rewrite-scope pin it implies, so the filter-less path is only being moved onto it. Also adds the assertion the suite never had: a remote-query check on a query with no WHERE clause. Every existing one filters, which is exactly why this survived. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
morningman
changed the title
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28346 ms |
Contributor
TPC-DS: Total hot run time: 168190 ms |
Contributor
ClickBench: Total hot run time: 23.84 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Related PR: #64304 (catalog SPI)
Problem Summary:
A connector never decides which columns to read — it renders whatever list
ConnectorScanRequest.getColumns()carries. The jdbc connector turns that list verbatim into the remoteSELECTlist and falls back toSELECT *when it is empty (JdbcQueryBuilder#buildQuery). So column pruning for every plugin-driven external scan rests entirely onPluginDrivenScanNode#buildColumnHandles(), which intersects the connector's column handles with this scan's tuple slots.That method had no direct coverage, and its failure mode (projecting more columns than the query needs) is a pure performance regression that no result-comparing test can observe. The existing jdbc explain assertions all pass a column list and assert those same columns are present; none of them can fail on an over-wide projection they did not anticipate.
This PR started as coverage for that gap. The new coverage immediately found a real bug, so it now carries the fix as well.
1. The bug: the connector's scan properties are computed before column pruning
A plugin-driven scan asks its connector for one property bundle — the jdbc remote
SELECT, per-column dictionaries, file format, path partition keys — and caches it (cachedPropertiesResult/scanNodeProperties).That cache is first filled from
init():init()runs while the translator is still translating this scan — strictly before the project above it prunes the tuple down to the columns the query reads (updateScanSlotsMaterialization). Everything the connector derives from the projection at that point therefore describes the full table schema.The only thing that dropped the cache was
convertPredicate(), and only when there was a conjunct to push down. Queries with aWHEREclause were rebuilt from the pruned tuple by accident; filter-less ones kept the pre-pruning bundle. Result:This holds for every
WHERE-less query on any plugin-driven external table, not justcount(*).The scan itself was not affected —
getSplits()rebuilds the column handles from the final tuple, so the query actually sent to the source was already pruned. What was wrong is the reported remote query, and anything else a connector derives from the projection through this bundle (populateScanLevelParams,getFileAttributes). Reviewers of the iceberg connector may want to check the field-id dictionary applied inIcebergScanPlanProvider#populateScanLevelParams, which is built from the requested columns and, on a filter-less query, was built over the full schema.Fix: drop the cache in
doFinalize(), the first point at which the tuple is final. Every filtered query already exercises this rebuild path today — including the second MVCC-snapshot / rewrite-scope pin it implies — so the filter-less path is only being moved onto an already-exercised path.Why it survived since the SPI migration: all 13 remote-query assertions in the tree filter, and a filter is exactly what used to hide this. The one filter-less assertion that exists (
test_gbase_jdbc_catalog, commented out) expects the pruned single column, i.e. the behavior this restores.2. The projection decision itself (
fe-core)PluginDrivenScanNodeColumnPruningTestdrives the realbuildColumnHandles()and pins:SELECT *fallback.Every assertion was mutation-checked against the production method: returning
allHandles.values()kills 4 of the 5, and making the unmatched-slot path fail loud unconditionally kills the 5th.3. Explain assertions (
external_table_p0)Two additions to
test_mysql_jdbc_catalog, both of which fail without the fix above:select k8 from test1) — the shape no existing assertion covered, and the most direct pin for the caching bug;count(*), the one shape whose projection would otherwise go empty. The engine keeps a single smallest slot (PhysicalPlanTranslator#updateScanSlotsMaterialization) instead of letting the tuple go empty, and an empty tuple is exactly what makes the jdbc connector emitSELECT *. The assertion pins that the remote select list stays one column wide and is not*; it deliberately does not pin which column wins, since that isgetSmallestSlot's business and tracks type widths.Release note
Fix
EXPLAINreporting a full-width remote read for queries without aWHEREclause on external catalogs backed by the connector plugin framework (e.g. jdbc). The scan itself already read only the projected columns.Check List (For Author)
Test
Behavior changed:
EXPLAINon a plugin-driven external scan without aWHEREclause now reports the pruned column list instead of the full schema. Query results and the SQL actually sent to the source are unchanged.Does this need documentation?
Check List (For Reviewer who merge this PR)