[improvement](hive) Push partition filters to HMS - #67725
Open
zhaorongsheng wants to merge 15 commits into
Open
zhaorongsheng wants to merge 15 commits into
zhaorongsheng wants to merge 15 commits into
Conversation
zhaorongsheng
requested review from
924060929,
CalvinKirs,
englefly,
morrySnow and
starocean999
as code owners
September 9, 2026 09:10
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
zhaorongsheng
force-pushed
the
codex/hms-partition-filter-pruning-master
branch
from
September 9, 2026 09:24
f2cc31c to
f563337
Compare
Contributor
|
/review |
Contributor
|
Thanks for working on this. Pushing selective Hive partition predicates to HMS is the right direction, but I found one merge blocker and a few framework-boundary issues that should be clarified before merging. P1: the current head does not compile. In .or(() -> Optional.ofNullable(SortedPartitionRanges.build(nameToPartitionItem)));Java only allows a lambda to capture a final or effectively-final local variable. A focused FE build on The build reached The deferred partition state should be represented explicitly.
Relatedly, There is also duplicated HMS work. The logical pruning path calls I do not think this PR needs to redesign the entire external-table predicate/residual framework. The broader work to unify partition pruning, updated connector handles, and per-conjunct residual tracking can be a maintainer follow-up. However, the compile failure, identity-only deferred state, and synthetic total count are introduced by this change and should be addressed here. If the duplicate HMS RPC is intentionally left as a follow-up, please document it and add a tracking issue, ideally with evidence that the selective path still materially improves planning latency. |
Contributor
|
A suggested long-term architecture, for clarity only — I do not think this full refactor should be required in this PR: The generic planner should model connector partition pruning as an explicit result-producing operation rather than requiring every engine to materialize the same eager The important properties would be:
This would support different external engines without adding engine-specific branches to Nereids or coupling logical pruning state to |
zhaorongsheng
force-pushed
the
codex/hms-partition-filter-pruning-master
branch
2 times, most recently
from
September 9, 2026 10:58
a88537e to
17e01cb
Compare
Contributor
Author
|
Thanks for the detailed review. Addressed in the latest head:
The duplicate connector partition-pruning RPC is intentionally left out of this focused fix and is tracked in #67739. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes for eight substantiated issues found on the fixed PR head. The central selective-partition goal is not achieved on the production Hive binding path, several early/parallel consumers mis-handle the new deferred state, and the connector API/dialect boundaries are incomplete.
Checkpoint conclusions:
- Goal and test proof: not satisfied. Production binding enumerates the full partition view before the deferred initializer, and common integral predicates fall back to full listing. The added tests are fake/helper-level and miss these production paths; the existing SPI surface test is statically guaranteed to fail.
- Scope and focus: the diff is thematically focused, and there was no additional user-provided focus. The full 17-file change was reviewed.
- Concurrency and lifecycle: physical finalization materializes no-filter deferred scans before batch selection/dispatch, so that path is ordered correctly. However, the preload path now leaves full Hive partition materialization and the live filtered RPC under internal-table read locks, and async-MV collection consumes the deferred-empty map too early.
- Configuration: no new setting is introduced, but enable_preload_external_metadata no longer warms this partition view before locks.
- Compatibility: the public ConnectorCapability addition lacks the mandatory frozen-surface baseline and API-major bump. Hive 1/2/3 database addressing and Hive partition-name escaping were checked and matched their established paths.
- Parallel and special paths: synchronous and partition-batch planning, zero matches, non-partition predicates, unsupported filter fallback, metadata add/delete, SQL block rules, and no-filter scans were traced. The original residual preserves row filtering on stable metadata; the accepted mixed-generation range and predicate-flag defects remain.
- Coverage and results: no builds or tests were run, as required by the review environment. Changed tests do not cover production MVCC binding, no-filter MV rewrite, real typed HMS parsing, cache-generation mixing, SQL block rules, preload lock scope, or the SPI version gate.
- Observability: unknown totals render safely as ?, and HMS fallback is logged. One accepted path nevertheless marks a known full view unknown.
- Persistence and FE/BE propagation: no transaction, EditLog, failover, storage-write, or BE protocol change is involved; the state/filter changes are FE-only and are propagated through physical translation.
- Performance: the production pre-bind list, table-wide sorted-range cache, integral-literal fallback, and non-partition-filter full-view rebuild each undermine the stated optimization.
- Residual review status: all candidates were accepted, merged, or dismissed with evidence. After Round 1 found the issues below, all normal and risk-focused Round 2 reviewers returned NO_NEW_VALUABLE_FINDINGS. The review is converged on this exact head.
zhaorongsheng
force-pushed
the
codex/hms-partition-filter-pruning-master
branch
from
September 10, 2026 01:59
17e01cb to
833beea
Compare
Contributor
Author
|
@924060929 would you mind reviewing it again? thanks ～ |
zhaorongsheng
force-pushed
the
codex/hms-partition-filter-pruning-master
branch
from
September 10, 2026 02:27
833beea to
3694b09
Compare
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes for five distinct issues on the exact reviewed head. The remote HMS pruning path works for stable supported integral/string equality and IN predicates, but the lightweight partition view is treated as authoritative by MTMV and fallback/finalization consumers; the direct HMS result is also unbounded, and batch lookup depends on a locally reconstructed identity.
Checkpoint conclusions:
- Goal and test proof: not satisfied. Supported EQ/IN filters can avoid the full name listing, but MAIN-1 through MAIN-5 leave destructive MTMV behavior, false partition states, an unsafe large-result path, and a batch-only correctness gap.
- Focus and smallness: no additional user focus was supplied. The 24-file patch is cohesive around Hive partition pruning, although its new partition-state representation leaks into unrelated MTMV and physical-scan consumers.
- Concurrency, lifecycle, and configuration: no new shared-state race, lock order, static lifecycle, setting, or dynamic-update issue survived. The already reported remote-I/O-under-plan-lock issue remains duplicate-fenced by r3968743408.
- Compatibility and rolling behavior: connector API 8.0 and its generated surface are aligned. Hive 1/2/3 database encoding, filter hooks, auth/TCCL, pool tainting, and failure propagation follow established paths. There is no FE-BE wire, variable, persistence, transaction, failover, or data-write change.
- Conditional and parallel paths: supported EQ/IN extraction is a safe necessary-condition widening and the original predicate remains as a residual. Unsupported predicates decline safely at the connector boundary, but the intended local fallback is broken by MAIN-3. Synchronous, partition-batch, no-filter, zero-match, external-mutation, SQL-block, EXPLAIN, query-partition-collection, and MTMV paths were traced. The concrete scan topology reviewed was LogicalFilter -> PruneFileScanPartition -> connector applyFilter/HMS -> PartitionPruner -> PhysicalFileScan -> PluginDrivenScanNode -> HiveScanPlanProvider; MTMV follows MTMVTask.beforeMTMVRefresh -> loadSnapshot -> alignMvPartition.
- Tests and results: the added tests are deterministic but fake/helper focused; they do not exercise the production MVCC subclass, MTMV alignment, connector-declined fallback, real large HMS response, or canonical-name round trip. No builds or tests were run because the authoritative review environment explicitly prohibits them.
- Observability and performance: existing fallback/cardinality logging is adequate as a standalone concern. MAIN-2 and MAIN-3 disable the intended batch path, and MAIN-4 bypasses the established adaptive metadata batching. The known duplicate logical/physical filter RPC is already acknowledged by follow-up #67739 and is not repeated here.
- Completion: all changed files and all producer/consumer paths for the new capability and deferred state were swept. Existing threads were treated as hard duplicate fences. After the first-round findings were adjudicated, both independent full-PR Round 2 reviewers and the risk-focused challenger returned NO_NEW_VALUABLE_FINDINGS, so this review is converged on head 3694b09.
| // MVCC table class because the catalog also serves snapshot-capable sibling formats, but its latest | ||
| // pin is deliberately not a data snapshot and applySnapshot is a no-op. Keep only that lightweight | ||
| // query-begin pin here; PruneFileScanPartition will request the selected partition view later. | ||
| if (supportsConnectorPartitionPruning()) { |
Contributor
There was a problem hiding this comment.
[P1] Do not expose this empty query pin to MTMV partition alignment. Scheduled MTMV refresh stores this exact result in MTMVTask.beforeMTMVRefresh, then alignMvPartition(mtmv, snapshots) asks the related Hive table for partition items using the pin. getNameToPartitionItems returns the empty map unchanged, so the related partition set is empty and alignment schedules every existing MTMV partition for deletion (creation likewise generates no partitions). Keep the lightweight state query-specific, or lazily materialize the full view for MTMV partition/freshness callers, and add a partitioned-Hive MTMV refresh test.
| PluginDrivenExternalTable table = (PluginDrivenExternalTable) getTargetTable(); | ||
| Optional<MvccSnapshot> snapshot = MvccUtil.getSnapshotFromContext(table, | ||
| Optional.ofNullable(getQueryTableSnapshot()), Optional.ofNullable(getScanParams())); | ||
| Map<String, PartitionItem> partitions = table.getNameToPartitionItems(snapshot); |
Contributor
There was a problem hiding this comment.
[P1] Materialize from a real full partition view here, not the deferred MVCC pin. For production Hive, the statement snapshot is the empty-map pin created by materializeLatest, and dynamic dispatch to PluginDrivenMvccExternalTable.getNameToPartitionItems(snapshot) simply returns that empty map. This therefore produces a materialized 0/0 selection while resolveRequiredPartitions still scans all partitions: batch mode is disabled and partition-count EXPLAIN/SQL-block enforcement sees zero for a full scan. The new unit test hides this by passing a fabricated nonempty map directly. Add a rehydration path that bypasses the lightweight pin and cover the real MVCC subclass.
| if (!connectorFilteredPartitions && nameToPartitionItem.isEmpty() | ||
| && (scan.getSelectedPartitions().isNotPruned() | ||
| || scan.getSelectedPartitions().isDeferredPartitionPruning())) { | ||
| nameToPartitionItem = externalTable.getNameToPartitionItems( |
Contributor
There was a problem hiding this comment.
[P1] Make this a real full-view fallback for the MVCC Hive subclass. For example, WHERE year > 2024 converts to a connector expression, but Hive extracts no equality/IN predicate and declines applyFilter; this line then dynamically dispatches to PluginDrivenMvccExternalTable.getNameToPartitionItems, which returns the preloaded lightweight pin's empty map. The rewrite consequently materializes a 0/0 selection: the scan still reads all partitions, but local pruning/batching and partition-count enforcement are lost, require_partition_filter rejects the valid predicate, and QueryPartitionCollector records zero used partitions. Use an explicit full-list path when connector pruning is declined and cover a non-equality Hive partition predicate on the real MVCC table.
|
|
||
| @Override | ||
| public List<HmsPartitionInfo> listPartitionsByFilter(String dbName, String tableName, String filter) { | ||
| return execute(client -> client.listPartitionsByFilter(dbName, tableName, filter, (short) -1).stream() |
Contributor
There was a problem hiding this comment.
[P1] Bound the cardinality of this full-partition response. A valid but low-selectivity predicate can match hundreds of thousands of partitions, and -1 asks HMS to serialize every storage descriptor in one Thrift reply; this bypasses the existing 5,000-item HmsPartitionBatchExecutor and its frame/message-size backoff, so planning can hit a frame limit or exhaust FE heap before the fallback helps. Request a safe threshold plus one and fall back to the name + adaptive-batch path when it is saturated (or otherwise paginate), and add a large-match test.
| if (index != 0) { | ||
| result.append('/'); | ||
| } | ||
| result.append(escapePathName(partKeys.get(index).toLowerCase(Locale.ROOT))) |
Contributor
There was a problem hiding this comment.
[P2] Preserve the metastore's canonical partition identity instead of rebuilding it with a different locale. Hive's FileUtils.makePartName lowercases keys using the metastore JVM's default locale, while this uses Locale.ROOT; with a Turkish-locale HMS and an API-created uppercase I partition key, Hive renders dotless ı=value but Doris batches i=value. getExistingPartitionsWithStats allows missing names, so batch mode silently omits that matching partition. Carry the native filtered partition metadata/name into batch planning (or obtain a server-rendered name) and add a locale-sensitive round-trip test.
zhaorongsheng
force-pushed
the
codex/hms-partition-filter-pruning-master
branch
from
September 10, 2026 07:22
3694b09 to
f601804
Compare
Contributor
|
For the next revision, I suggest running the repository review workflow locally before requesting another remote https://github.com/apache/doris-skills/blob/main/skills/doris-repo-review/SKILL.md A practical iteration loop is: Two details are important:
This local loop should catch cross-module call-chain, lifecycle, compatibility, and test-coverage problems before consuming another remote CI/reviewer round. |
Contributor
Author
|
@924060929 I have run /doris-repo-review and local tests every loop. Would you mind reviewing it again? Thanks |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Review status: changes requested — two distinct blockers remain on f601804000e0f6def70097a025b2b6c76f81e428. Existing review issues were treated as duplicate fences and are not repeated here.
Critical-checkpoint conclusions:
- Goal and proof: The PR does route supported Hive equality/IN partition predicates to HMS and retains conservative local fallbacks, but it does not fully preserve the prior planning behavior: large filtered scans lose streaming batch discovery, and MTMV full-view repair moves connector work under metadata locks. The added unit tests prove helper/state mechanics, not either production interaction.
- Scope/focus: The change is coherent and confined to Hive connector pruning plus the required FE deferred-view lifecycle and SPI surface.
review_focus.txtsupplied no additional focus. - Concurrency: No new worker ownership or mutable-data race was found, and the existing table-lock ordering remains sorted. However, both scheduled-MTMV mapping phases can now perform remote/O(all partitions) work while internal-table read locks are retained; see the inline blocker.
- Lifecycle:
NOT_PRUNED,DEFERRED, andMATERIALIZEDwere traced through preload, binding, pruning, collection, finalization, counts, scan dispatch, MV compensation, and MTMV freshness. Current no-filter, unsupported-filter, and zero-match terminal states are coherent; the MTMV materialization placement is not. - Configuration: No configuration item is added or changed.
- Compatibility: Connector capability, public-surface baseline, module resource filtering, and API major
8.0are consistent. No storage-format or FE/BE wire change is introduced. - Parallel and conditional paths: Equality/IN, range/unsupported, no-filter, zero-match, saturation fallback, transactional Hive, logical/physical filtering, synchronous scan, batch scan, and Hive-version database routing were checked. The new filtered-handle batch condition creates the second inline blocker; the known double-filter generation issue remains the acknowledged #67739 follow-up.
- Tests and results: The Java tests cover mocked HMS filtering, saturation, state transitions, counts, and helper gates. They omit a production MTMV lock-timing test and a greater-than-threshold filtered split-streaming test. No builds or tests were run during this review-only task, and no generated regression result file is involved.
- Observability: Existing pruning logs and batch profile statistics provide the relevant identifiers/counts; no separate metrics gap was substantiated.
- Persistence, transactions, and writes: No EditLog, persisted-state, transaction, data-write, or crash-recovery behavior changes.
- FE/BE variables: No new variable or payload crosses the FE/BE boundary.
- Performance: Selective remote pruning is beneficial, but the two accepted findings introduce avoidable lock-held full enumeration and synchronous large-result split materialization.
- Other issues: A final 27-file sweep and a fresh convergence round found no additional distinct issue beyond these comments, existing inline threads, and #67739.
| // predicate to HMS first. Consumers that explicitly ask for a partition map (MTMV alignment, | ||
| // no-filter scan finalization, and a connector-declined pruning fallback) require the real full | ||
| // view instead of treating that query-only pin as an empty table. | ||
| return super.getNameToPartitionItems(snapshot); |
Contributor
There was a problem hiding this comment.
[P1] Rehydrate this MTMV partition view before taking base-table locks. MTMVTask.beforeMTMVRefresh now stores only the lightweight empty pin, but syncPartitionsIfNeeded acquires every base table's read lock before alignMvPartition reaches this live full-view call; buildRefreshContext repeats the same path under the same locks. For an MV joining Hive with an internal table, a slow or very large HMS listing therefore blocks metadata writers/DDL on the internal table for the full RPC and O(all partitions) map construction. This is distinct from the existing Nereids planWithLock thread: it is the scheduled MTMV refresh path introduced while fixing the empty-pin correctness issue. Capture and reuse the full Hive view before MetaLockUtils.readLockTables (or otherwise keep connector I/O outside that scope), and add a lock-timing regression test.
| // scan path avoids reconstructing partition names from key/value pairs and fetching them again per batch, | ||
| // which can lose the metastore's locale-sensitive canonical key spelling. | ||
| return partKeyNames != null && !partKeyNames.isEmpty() && !hiveHandle.isTransactional() | ||
| && hiveHandle.getPrunedPartitions() == null; |
Contributor
There was a problem hiding this comment.
[P1] Keep large connector-pruned scans eligible for batched split generation. Physical applyFilter installs this pruned handle before the generic 1,024-partition gate runs, so returning false here makes planScan synchronously list and split every surviving partition before producing any work. A direct HMS result can contain 5,000 partitions, and the saturation fallback can materialize an arbitrarily larger matched subset, so a valid low-selectivity predicate can regress from bounded asynchronous batches to a long planning stall and large FE allocation. Preserve the metastore-native partition objects/identity in a batchable representation and add a greater-than-threshold filtered-scan test that proves split discovery is streamed.
zhaorongsheng
force-pushed
the
codex/hms-partition-filter-pruning-master
branch
from
September 10, 2026 10:48
f601804 to
2cb618e
Compare
Contributor
|
/review |
zhaorongsheng
force-pushed
the
codex/hms-partition-filter-pruning-master
branch
from
September 10, 2026 10:53
2cb618e to
8ca2569
Compare
Contributor
|
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
zhaorongsheng
force-pushed
the
codex/hms-partition-filter-pruning-master
branch
from
September 11, 2026 06:09
8ca2569 to
4d1911e
Compare
Contributor
Author
|
@924060929 Please review it once again, thanks~~ |
zhaorongsheng
force-pushed
the
codex/hms-partition-filter-pruning-master
branch
from
September 16, 2026 02:36
45798d5 to
4be7355
Compare
zhaorongsheng
pushed a commit
to zhaorongsheng/doris
that referenced
this pull request
Sep 16, 2026
### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: The direct-HMS partition-pruning path applied the connector predicate twice - once while logical pruning built the Nereids partition selection, and again on a freshly resolved handle in the physical scan. A metastore change between the two calls could therefore mix the selected partition names with another generation's partition metadata, and the batched split path (which resolves those names through the handle's own pruned-partition map) threw for a partition that had disappeared. The saturation check also ran on the hook-filtered list size even though the metastore filter hook is applied after the raw page cap, so one hook-hidden entry could make a truncated page look complete and silently drop matching partitions. Deferred partition materialization enumerated the latest handle rather than the statement's pinned one, and built every item from the raw per-column value map without the connector's SQL-NULL flags, so a single unrepresentable typed value aborted the query instead of disabling pruning. Fixes: carry the logical filter result (handle plus remaining filter) into physical planning and reuse it; decide HMS saturation on the raw pre-hook page size, which the vendored client now reports; pin the MVCC snapshot before materializing a partition view (filtered and unfiltered); and share the source-agnostic, NULL-flag-aware partition-item builder with a degrade-to-scan-all contract for the scan paths. The HMS filter logging remains count-based as fixed previously. ### Release note None ### Check List (For Author) - Test: Unit Test (fe-core 117 tests: PluginDrivenMvccExternalTableTest 74, PluginDrivenScanNodeBatchModeTest 14, PluginDrivenScanNodePartitionPruningTest 11, PluginDrivenScanNodePartitionCountTest 9, PluginDrivenExternalTablePartitionTest 9; connectors 45 tests: HiveConnectorMetadataPartitionPruningTest 19, HiveScanBatchModeTest 20, ThriftHmsClientMaxPartsTest 6). Also ran the full FE Maven reactor build (`mvn -f fe/pom.xml package -DskipTests -Dskip.doc=true`), BUILD SUCCESS. - Behavior changed: Yes (the connector predicate is applied once per scan; HMS filter saturation is decided on the raw pre-hook page; deferred partition views are materialized from the pinned snapshot and degrade to scan-all instead of failing) - Does this need documentation: No
zhaorongsheng
pushed a commit
to zhaorongsheng/doris
that referenced
this pull request
Sep 16, 2026
### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Pushing partition predicates to the connector changed HiveConnectorMetadata.listPartitions(..., filter): a filter carrying partition equality / IN predicates is now resolved before the partition view is built, and a client that serves no HMS filter dialect falls back to listing names locally and fetching only the surviving partitions by name. Two pre-existing unit tests still encoded the previous "the filter is ignored and listing is names-only" contract, so they failed once the filter path became live. testFilterIsIgnored becomes testPartitionPredicateResolvesThroughLocalFallback and asserts the pruned result plus the by-name fetch of the survivors, and both test fakes echo the requested partitions instead of failing loud, so the unfiltered tests that assert get_partitions_by_names was never called keep their guard. ### Release note None ### Check List (For Author) - Test: Unit Test (fe-connector-hive 457, plus fe-connector-hms / fe-connector-spi suites; `mvn -f fe/pom.xml -pl :fe-connector-hive,:fe-connector-hms,:fe-connector-spi -am test` BUILD SUCCESS) - Behavior changed: No (test-only change) - Does this need documentation: No
zhaorongsheng
pushed a commit
to zhaorongsheng/doris
that referenced
this pull request
Sep 16, 2026
### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Four review follow-ups on the connector partition pruning path. (1) The eager partition-view helper still carried a filter parameter that only the removed filtered shape used, leaving an unreachable pin branch and a comment describing a decision that can no longer differ; the helper is inlined and the pin is now unconditional (the filtered shape is served by applyPartitionFilterForScan). (2) The vendored metastore client read the raw page size inline from the same list it had just handed to the configurable filter hook, so a hook that filters in place would make the "raw" count equal the post-hook count and re-introduce the silent truncation the raw-count contract exists to prevent; the count is captured before the hook runs. (3) Several engine/connector comments still attributed the NULL-flag-aware partition-item builder to its old owner after the method moved to PluginDrivenExternalTable. (4) The partition-view-cache test fake stopped failing loud on get_partitions_by_names without any assertion replacing that guard, so the file no longer protected the names-only listing invariant; the fake records the call again and a new test pins both shapes (unfiltered: names only, filtered: survivors fetched by name). ### Release note None ### Check List (For Author) - Test: Unit Test (targeted classes: fe-core 117, fe-connector-hive 54, fe-connector-hms 6, fe-connector-paimon 16, fe-connector-iceberg 63) plus the full FE Maven reactor build (`mvn -f fe/pom.xml package -DskipTests -Dskip.doc=true`), BUILD SUCCESS. - Behavior changed: No (the only functional tightening is the raw-count capture order, which differs only for a filter hook that mutates its input list) - Does this need documentation: No
zhaorongsheng
pushed a commit
to zhaorongsheng/doris
that referenced
this pull request
Sep 16, 2026
… selection ### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Three issues from the latest review of the connector-pruning path. (1) The batch resolver treated the retained native partition map as if it covered the whole logical selection and threw for any name it lacked; the map only holds what the connector predicate admitted, and the two converters can disagree (CAST(p AS INT) = 1 is declined by the connector converter while the physical converter strips it to p = '1'), so a typed logical prune may legitimately select a name the map does not hold. A batch is now resolved by name exactly as the pre-cutover path did whenever the map does not cover it, so one consistent generation is read and no selected partition is rejected. (2) The local name prefilter's result is reused as the logical selected view, but it compared rendered values as text, so a type-equal but textually different value (INT 01 versus predicate 1) was dropped before the typed pruner ran - and, in batch mode, omitted from the scan. Integral keys are now compared numerically (STRING and every other type keep exact text comparison, so p=1 and p=01 remain distinct partitions there). (3) A partition key named after a metastore filter keyword (for example date) or consisting only of digits was accepted as a filter identifier, although the grammar lexes it as a keyword or an integral literal where a key operand must be an Identifier; the direct path is now declined for those names instead of sending a request that can only fail, taint the pooled client, and fall back to a full enumeration. ### Release note None ### Check List (For Author) - Test: Unit Test (fe-connector-hive 461 tests, `mvn -f fe/pom.xml -pl :fe-connector-hive -am test`, BUILD SUCCESS). New cases: batch fallback when the retained map covers fewer names than the logical batch, numeric comparison of integral partition values under the local fallback, and declining reserved-word / all-digit partition keys. - Behavior changed: Yes (batch resolution falls back to name lookup when the native map does not cover the batch; integral partition values compare numerically; reserved-word and all-digit partition keys no longer attempt the HMS filter RPC) - Does this need documentation: No
zhaorongsheng
pushed a commit
to zhaorongsheng/doris
that referenced
this pull request
Sep 16, 2026
…ensation ### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: A connector that materializes a partition predicate remotely - the hive HMS filter pushdown this PR adds - leaves a file scan in the DEFERRED partition state until something enumerates it: PruneFileScanPartition only materializes the selection when a partition predicate reaches it, and PluginDrivenScanNode materializes a no-predicate full scan in its own finalize. Taken as it reads, that state means "not enumerated yet", but QueryPartitionCollector turned it into PartitionCompensator.ALL_PARTITIONS, i.e. "this query reads every partition of the base table", and the compensator reads that marker as "the materialized view already covers everything, so no union compensation is needed" (needUnionRewrite returns false before any invalid partition is computed). The marker therefore suppressed exactly the compensation that re-reads the base partitions an MV does not cover: once a partition had been added to a hive base table, a rewritten query silently lost its rows although the partition union was expected. `nereids_rules_p0.mv.external_table.part_partition_invalid` failed on the partition added after the last refresh, and `mtmv_p0.test_hive_rewrite_mtmv` stopped emitting the VUNION for its un-refreshed partition. The collector now asks the connector for the scan's (unfiltered) partition view and records those names, keeping the marker only when that view is unavailable - the same input the pre-deferral code collected, and the same view the scan itself has to materialize before generating splits, served from the connector's partition view cache. DEFERRED is produced by PluginDrivenExternalTable.initSelectedPartitions alone, so the connector is the only authority for the names such a scan reads. ### Release note None ### Check List (For Author) - Test: Unit Test (targeted fe-core classes: PluginDrivenScanNode{PartitionCount,PartitionPruning,BatchMode,SysTableGuard,Compatibility}Test 62, PluginDrivenExternalTablePartitionTest 9, PluginDrivenMvccExternalTableTest 74; BUILD SUCCESS). The behaviour itself is asserted by the two external suites this fixes (nereids_rules_p0.mv.external_table.part_partition_invalid, mtmv_p0.test_hive_rewrite_mtmv), which need a Hive metastore and are not run locally. - Behavior changed: Yes (an MV-rewrite candidate query over a deferred hive scan now reports the scan's real partition names instead of "all partitions", which restores partition union compensation) - Does this need documentation: No
zhaorongsheng
pushed a commit
to zhaorongsheng/doris
that referenced
this pull request
Sep 16, 2026
…ection ### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Two scan-node defects of the connector-pruning path. (1) A connector-filtered selection - the shape PruneFileScanPartition builds once HMS accepted the predicate - carries only the surviving partition names, since not enumerating the table's full partition view is precisely what the pushdown buys, so it cannot report the table's TOTAL partition count and left it at UNKNOWN_TOTAL_PARTITION_NUM, which the EXPLAIN renderers print as `?`. Every EXPLAIN of a partitioned hive table whose partition predicate is an equality or an IN therefore changed from `partition=N/M` to `partition=N/?` (external_table_p0.hive.test_hive_default_partition asserts values such as `partition=1/4` and `partition=2/4` in exactly those cases), losing the number readers use to judge how much the predicate pruned. The total is now resolved from the connector's UNFILTERED partition view where the line is rendered - the same view a no-filter full scan already materializes on this node before generating splits - so a statement that renders no EXPLAIN string still pays nothing for the enumeration, and neither the user-visible format nor any regression assertion changes; an unavailable view keeps `?` instead of a fabricated 0. (2) materializeDeferredSelectedPartitions dereferenced the partition selection unconditionally, although this node treats a null selection as "nothing selected" everywhere else (resolveRequiredPartitions, displayPartitionCounts, shouldUseBatchMode, numApproximateSplits); that inconsistency NPE'd finalize for an unset selection and broke the existing master test PluginDrivenScanNodeCompatibilityTest.compatibilityCheckRunsOnlyAfterScanSlotsAreFinalized, which finalizes a scan node whose selection was never set. ### Release note None ### Check List (For Author) - Test: Unit Test (fe-core targeted classes, 65 tests, BUILD SUCCESS): PluginDrivenScanNodePartitionCountTest 12 (3 new cases pin the total resolution and the `?` fallback), PluginDrivenScanNodeCompatibilityTest 8 (red before the null guard), PluginDrivenScanNodePartitionPruningTest 11, PluginDrivenScanNodeBatchModeTest 14, PluginDrivenScanNodeSysTableGuardTest 17, FileCacheAdmissionRuleRefresherTest 3; plus a full FE reactor build (`mvn -f fe/pom.xml -pl :fe-core -am test-compile` with checkstyle, 0 violations). The connector-filtered EXPLAIN itself needs a Hive metastore, so the assertions it restores (external_table_p0.hive.test_hive_default_partition) are verified by the external-regression job. - Behavior changed: Yes (EXPLAIN of a connector-filtered hive scan again shows the table's real total partition count; an unset partition selection no longer NPEs the scan node's finalize) - Does this need documentation: No
added 8 commits
September 16, 2026 10:37
Issue Number: close apache#67724 Related PR: apache#67739 Problem Summary: Hive partition pruning could enumerate every HMS partition before a selective predicate reached the connector. This change keeps the plain-Hive latest snapshot lightweight, materializes a remote partition view only after the connector accepts the predicate, and retains full-list local pruning as the compatibility fallback. It preserves native filtered partition objects across asynchronous batch split planning, so large selective scans remain streamed without reconstructing a metastore partition name. It also materializes the complete Hive view for MTMV before base-table locks are acquired, allowing lock-held alignment and refresh-context construction to reuse the pinned map. Improve planning latency for selective Hive partition queries when HMS supports get_partitions_by_filter. - Test: Unit Test - HiveScanBatchModeTest, HiveConnectorMetadataPartitionPruningTest, ThriftHmsClientMaxPartsTest (43 tests) - PluginDrivenMvccExternalTableTest (71 tests) - DISABLE_BUILD_UI=ON ./build.sh --fe - git diff --check - Behavior changed: Yes (selective Hive partition predicates use a connector-filtered view while large results retain asynchronous batch split discovery and MTMV materializes its view before metadata locks) - Does this need documentation: No
### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: HMS partition filter pruning logged the complete generated predicate at INFO and WARN. A large IN predicate could therefore produce unbounded hot logs and expose partition values; the expected saturated-response fallback also emitted a WARN stack trace. Log only the predicate value count at INFO/WARN, retain a 256-character debug-only summary, and use a dedicated saturation exception so the expected fallback is logged without a stack trace. ### Release note None ### Check List (For Author) - Test: Unit Test (attempted `mvn -f fe/pom.xml -pl :fe-connector-hive,:fe-connector-hms -am test -Dtest=HiveConnectorMetadataPartitionPruningTest,ThriftHmsClientMaxPartsTest -DfailIfNoTests=false`; blocked because local Maven 3.8.6 does not meet the required >= 3.9.0) - Behavior changed: Yes (HMS filter log payload and expected saturation log level) - Does this need documentation: No
### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: The direct-HMS partition-pruning path applied the connector predicate twice - once while logical pruning built the Nereids partition selection, and again on a freshly resolved handle in the physical scan. A metastore change between the two calls could therefore mix the selected partition names with another generation's partition metadata, and the batched split path (which resolves those names through the handle's own pruned-partition map) threw for a partition that had disappeared. The saturation check also ran on the hook-filtered list size even though the metastore filter hook is applied after the raw page cap, so one hook-hidden entry could make a truncated page look complete and silently drop matching partitions. Deferred partition materialization enumerated the latest handle rather than the statement's pinned one, and built every item from the raw per-column value map without the connector's SQL-NULL flags, so a single unrepresentable typed value aborted the query instead of disabling pruning. Fixes: carry the logical filter result (handle plus remaining filter) into physical planning and reuse it; decide HMS saturation on the raw pre-hook page size, which the vendored client now reports; pin the MVCC snapshot before materializing a partition view (filtered and unfiltered); and share the source-agnostic, NULL-flag-aware partition-item builder with a degrade-to-scan-all contract for the scan paths. The HMS filter logging remains count-based as fixed previously. ### Release note None ### Check List (For Author) - Test: Unit Test (fe-core 117 tests: PluginDrivenMvccExternalTableTest 74, PluginDrivenScanNodeBatchModeTest 14, PluginDrivenScanNodePartitionPruningTest 11, PluginDrivenScanNodePartitionCountTest 9, PluginDrivenExternalTablePartitionTest 9; connectors 45 tests: HiveConnectorMetadataPartitionPruningTest 19, HiveScanBatchModeTest 20, ThriftHmsClientMaxPartsTest 6). Also ran the full FE Maven reactor build (`mvn -f fe/pom.xml package -DskipTests -Dskip.doc=true`), BUILD SUCCESS. - Behavior changed: Yes (the connector predicate is applied once per scan; HMS filter saturation is decided on the raw pre-hook page; deferred partition views are materialized from the pinned snapshot and degrade to scan-all instead of failing) - Does this need documentation: No
### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Pushing partition predicates to the connector changed HiveConnectorMetadata.listPartitions(..., filter): a filter carrying partition equality / IN predicates is now resolved before the partition view is built, and a client that serves no HMS filter dialect falls back to listing names locally and fetching only the surviving partitions by name. Two pre-existing unit tests still encoded the previous "the filter is ignored and listing is names-only" contract, so they failed once the filter path became live. testFilterIsIgnored becomes testPartitionPredicateResolvesThroughLocalFallback and asserts the pruned result plus the by-name fetch of the survivors, and both test fakes echo the requested partitions instead of failing loud, so the unfiltered tests that assert get_partitions_by_names was never called keep their guard. ### Release note None ### Check List (For Author) - Test: Unit Test (fe-connector-hive 457, plus fe-connector-hms / fe-connector-spi suites; `mvn -f fe/pom.xml -pl :fe-connector-hive,:fe-connector-hms,:fe-connector-spi -am test` BUILD SUCCESS) - Behavior changed: No (test-only change) - Does this need documentation: No
### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Four review follow-ups on the connector partition pruning path. (1) The eager partition-view helper still carried a filter parameter that only the removed filtered shape used, leaving an unreachable pin branch and a comment describing a decision that can no longer differ; the helper is inlined and the pin is now unconditional (the filtered shape is served by applyPartitionFilterForScan). (2) The vendored metastore client read the raw page size inline from the same list it had just handed to the configurable filter hook, so a hook that filters in place would make the "raw" count equal the post-hook count and re-introduce the silent truncation the raw-count contract exists to prevent; the count is captured before the hook runs. (3) Several engine/connector comments still attributed the NULL-flag-aware partition-item builder to its old owner after the method moved to PluginDrivenExternalTable. (4) The partition-view-cache test fake stopped failing loud on get_partitions_by_names without any assertion replacing that guard, so the file no longer protected the names-only listing invariant; the fake records the call again and a new test pins both shapes (unfiltered: names only, filtered: survivors fetched by name). ### Release note None ### Check List (For Author) - Test: Unit Test (targeted classes: fe-core 117, fe-connector-hive 54, fe-connector-hms 6, fe-connector-paimon 16, fe-connector-iceberg 63) plus the full FE Maven reactor build (`mvn -f fe/pom.xml package -DskipTests -Dskip.doc=true`), BUILD SUCCESS. - Behavior changed: No (the only functional tightening is the raw-count capture order, which differs only for a filter hook that mutates its input list) - Does this need documentation: No
… selection ### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Three issues from the latest review of the connector-pruning path. (1) The batch resolver treated the retained native partition map as if it covered the whole logical selection and threw for any name it lacked; the map only holds what the connector predicate admitted, and the two converters can disagree (CAST(p AS INT) = 1 is declined by the connector converter while the physical converter strips it to p = '1'), so a typed logical prune may legitimately select a name the map does not hold. A batch is now resolved by name exactly as the pre-cutover path did whenever the map does not cover it, so one consistent generation is read and no selected partition is rejected. (2) The local name prefilter's result is reused as the logical selected view, but it compared rendered values as text, so a type-equal but textually different value (INT 01 versus predicate 1) was dropped before the typed pruner ran - and, in batch mode, omitted from the scan. Integral keys are now compared numerically (STRING and every other type keep exact text comparison, so p=1 and p=01 remain distinct partitions there). (3) A partition key named after a metastore filter keyword (for example date) or consisting only of digits was accepted as a filter identifier, although the grammar lexes it as a keyword or an integral literal where a key operand must be an Identifier; the direct path is now declined for those names instead of sending a request that can only fail, taint the pooled client, and fall back to a full enumeration. ### Release note None ### Check List (For Author) - Test: Unit Test (fe-connector-hive 461 tests, `mvn -f fe/pom.xml -pl :fe-connector-hive -am test`, BUILD SUCCESS). New cases: batch fallback when the retained map covers fewer names than the logical batch, numeric comparison of integral partition values under the local fallback, and declining reserved-word / all-digit partition keys. - Behavior changed: Yes (batch resolution falls back to name lookup when the native map does not cover the batch; integral partition values compare numerically; reserved-word and all-digit partition keys no longer attempt the HMS filter RPC) - Does this need documentation: No
…ensation ### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: A connector that materializes a partition predicate remotely - the hive HMS filter pushdown this PR adds - leaves a file scan in the DEFERRED partition state until something enumerates it: PruneFileScanPartition only materializes the selection when a partition predicate reaches it, and PluginDrivenScanNode materializes a no-predicate full scan in its own finalize. Taken as it reads, that state means "not enumerated yet", but QueryPartitionCollector turned it into PartitionCompensator.ALL_PARTITIONS, i.e. "this query reads every partition of the base table", and the compensator reads that marker as "the materialized view already covers everything, so no union compensation is needed" (needUnionRewrite returns false before any invalid partition is computed). The marker therefore suppressed exactly the compensation that re-reads the base partitions an MV does not cover: once a partition had been added to a hive base table, a rewritten query silently lost its rows although the partition union was expected. `nereids_rules_p0.mv.external_table.part_partition_invalid` failed on the partition added after the last refresh, and `mtmv_p0.test_hive_rewrite_mtmv` stopped emitting the VUNION for its un-refreshed partition. The collector now asks the connector for the scan's (unfiltered) partition view and records those names, keeping the marker only when that view is unavailable - the same input the pre-deferral code collected, and the same view the scan itself has to materialize before generating splits, served from the connector's partition view cache. DEFERRED is produced by PluginDrivenExternalTable.initSelectedPartitions alone, so the connector is the only authority for the names such a scan reads. ### Release note None ### Check List (For Author) - Test: Unit Test (targeted fe-core classes: PluginDrivenScanNode{PartitionCount,PartitionPruning,BatchMode,SysTableGuard,Compatibility}Test 62, PluginDrivenExternalTablePartitionTest 9, PluginDrivenMvccExternalTableTest 74; BUILD SUCCESS). The behaviour itself is asserted by the two external suites this fixes (nereids_rules_p0.mv.external_table.part_partition_invalid, mtmv_p0.test_hive_rewrite_mtmv), which need a Hive metastore and are not run locally. - Behavior changed: Yes (an MV-rewrite candidate query over a deferred hive scan now reports the scan's real partition names instead of "all partitions", which restores partition union compensation) - Does this need documentation: No
…ection ### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Two scan-node defects of the connector-pruning path. (1) A connector-filtered selection - the shape PruneFileScanPartition builds once HMS accepted the predicate - carries only the surviving partition names, since not enumerating the table's full partition view is precisely what the pushdown buys, so it cannot report the table's TOTAL partition count and left it at UNKNOWN_TOTAL_PARTITION_NUM, which the EXPLAIN renderers print as `?`. Every EXPLAIN of a partitioned hive table whose partition predicate is an equality or an IN therefore changed from `partition=N/M` to `partition=N/?` (external_table_p0.hive.test_hive_default_partition asserts values such as `partition=1/4` and `partition=2/4` in exactly those cases), losing the number readers use to judge how much the predicate pruned. The total is now resolved from the connector's UNFILTERED partition view where the line is rendered - the same view a no-filter full scan already materializes on this node before generating splits - so a statement that renders no EXPLAIN string still pays nothing for the enumeration, and neither the user-visible format nor any regression assertion changes; an unavailable view keeps `?` instead of a fabricated 0. (2) materializeDeferredSelectedPartitions dereferenced the partition selection unconditionally, although this node treats a null selection as "nothing selected" everywhere else (resolveRequiredPartitions, displayPartitionCounts, shouldUseBatchMode, numApproximateSplits); that inconsistency NPE'd finalize for an unset selection and broke the existing master test PluginDrivenScanNodeCompatibilityTest.compatibilityCheckRunsOnlyAfterScanSlotsAreFinalized, which finalizes a scan node whose selection was never set. ### Release note None ### Check List (For Author) - Test: Unit Test (fe-core targeted classes, 65 tests, BUILD SUCCESS): PluginDrivenScanNodePartitionCountTest 12 (3 new cases pin the total resolution and the `?` fallback), PluginDrivenScanNodeCompatibilityTest 8 (red before the null guard), PluginDrivenScanNodePartitionPruningTest 11, PluginDrivenScanNodeBatchModeTest 14, PluginDrivenScanNodeSysTableGuardTest 17, FileCacheAdmissionRuleRefresherTest 3; plus a full FE reactor build (`mvn -f fe/pom.xml -pl :fe-core -am test-compile` with checkstyle, 0 violations). The connector-filtered EXPLAIN itself needs a Hive metastore, so the assertions it restores (external_table_p0.hive.test_hive_default_partition) are verified by the external-regression job. - Behavior changed: Yes (EXPLAIN of a connector-filtered hive scan again shows the table's real total partition count; an unset partition selection no longer NPEs the scan node's finalize) - Does this need documentation: No
zhaorongsheng
force-pushed
the
codex/hms-partition-filter-pruning-master
branch
from
September 16, 2026 02:37
4be7355 to
040ff93
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17074 ms |
Contributor
|
/review |
Contributor
TPC-DS: Total hot run time: 83209 ms |
Contributor
ClickBench: Total hot run time: 14.89 s |
Contributor
There was a problem hiding this comment.
Static exact-head review of 040ff93f0c7a62a32640895181d9cd1735a6b1db against base 563d6e3727c7da2f876f6c652e94e48eb2a2b2d7. I reviewed all 38 changed paths in the authoritative diff and duplicate-fenced the 23 existing inline threads. This review adds three distinct findings: two P1 and one P2.
Key checkpoint conclusions:
- Goal/tests: the change pushes supported Hive partition predicates to HMS while preserving local/full-list fallback, snapshot consistency, batching, MV/MTMV semantics, and EXPLAIN counts. The added unit/surface tests cover the main states, but not uppercase fallback keys, unavailable-view MV accounting, or lock timing.
- Scope/parallel paths/conditions: traced direct HMS filtering, saturation/error fallback, unfiltered listing, logical-to-physical handle transport, batch planning, no-filter scans, MV compensation, MTMV rehydration, and EXPLAIN rendering, including zero-match, unavailable, deferred, and materialized states.
- Concurrency/lifecycle: snapshot application and ordinary state copies are coherent, but the new no-filter MV collector performs full external enumeration while planner table locks are held (inline P1), and unavailable scan-all state is misreported to MV accounting (inline P2).
- Compatibility/config: no new configuration contract, persistence format, data-write path, or FE-BE wire variable is introduced. Connector SPI surface/version checks and the retry-proxy/raw-page saturation path are consistent; Hive partition-name casing still breaks one local fallback path (inline P1).
- Observability/performance: bounded filter logging and saturation telemetry are adequate. The lock-held O(all partitions) materialization is the new performance/concurrency blocker; other unbounded-list and lock-scope cases are already covered by existing threads.
- Tests/results: static review only; I did not run builds or tests under the review contract. At submission time, GitHub showed FE unit tests, compile, and external regression passing, while the P0 regression check was failing and FE coverage was pending.
- Other checkpoints: no additional persistence, data-write, configuration, FE-BE, or unfenced correctness issue survived two bounded review rounds.
| for (Map.Entry<String, List<String>> entry : predicates.entrySet()) { | ||
| String colName = entry.getKey(); | ||
| List<String> allowedValues = entry.getValue(); | ||
| String actualValue = partValues.get(colName); |
Contributor
There was a problem hiding this comment.
[P1] Bind fallback values to the declared partition keys
Hive 3.1.3's FileUtils.makePartName lowercases partition keys, but this lookup uses the original declared spelling. For example, an API-created key P-X cannot use the direct HMS filter, so the local fallback parses Hive's p-x=1 into key p-x; this line then looks up P-X, rejects every real partition, and the carried selection makes the query return no rows. Please bind decoded values positionally to partKeyNames (and validate arity) instead of trusting the rendered key spelling, and add an uppercase special-key fallback test.
| Pair.of(catalogRelation.getRelationId(), tablePartitions)); | ||
| } | ||
| } else { | ||
| tablePartitions.addAll(selectedPartitions.selectedPartitions.keySet()); |
Contributor
There was a problem hiding this comment.
[P2] Treat unavailable NOT_PRUNED views as all partitions
This new non-DEFERRED branch assumes the selection map is authoritative, but PruneFileScanPartition also returns NOT_PRUNED when an unrepresentable connector partition makes the full view unavailable. Execution correctly interprets that state as scan-all, while this code records its empty sentinel map as a concrete zero-partition set; PartitionCompensator defines that as "query no partitions," so the async-MV path rejects an otherwise eligible rewrite. Please map the unavailable/scan-all state to ALL_PARTITIONS (or carry a distinct state) and test a connector-declined predicate with one unrepresentable typed partition.
| CascadesContext context) { | ||
| Optional<MvccSnapshot> snapshot = context.getStatementContext().getSnapshot(table, | ||
| scan.getTableSnapshot(), scan.getScanParams()); | ||
| Optional<Map<String, PartitionItem>> partitions = table.getNameToPartitionItemsForScan(snapshot); |
Contributor
There was a problem hiding this comment.
[P1] Materialize this view before taking planner table locks
afterRewrite runs after StatementContext.lock(), and an unfiltered Hive scan reaches this call with a cold DEFERRED view. The call then performs the full connector/HMS listing and builds every partition item while read locks for any internal tables in the same query remain held, blocking DDL and metadata writers for unbounded external latency and O(all partitions) work. This is a separate no-filter MV-collection path from the existing predicate-pruning lock thread. Please preload/reuse the pinned full view before locking, and cover a mixed internal/Hive MV-rewrite plan with a slow listing.
Contributor
FE Regression Coverage ReportIncrement line coverage |
added 7 commits
September 16, 2026 15:53
…loaded view ### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Three findings of the connector-pruning review, all on the paths that consume a connector-pruned partition view. (1) QueryPartitionCollector runs from InitMaterializationContextHook.afterRewrite, i.e. after StatementContext.lock(), and an unfiltered hive scan reaches it still DEFERRED, so the collector resolved the table's full partition view there: an unbounded HMS round-trip plus an O(all partitions) item build executed while the statement held read locks on every internal table of the same query, blocking their DDL and metadata writers for that whole window. The pre-lock preload pass - the phase that exists to do this work before the locks - now materializes that view for a connector-pruning table and records it on the table's preload entry, and the collector reuses it instead of asking the connector again; a reference carrying a version selector keeps enumerating its own generation, since the preload warms the latest one only. The preload switch is opt-in, so with it off the collector behaves exactly as before - the same enumeration the pre-deferral code performed at bind time under the same lock - and with it on the collector needs no connector call at all. (2) The same collector read a NOT_PRUNED selection as a concrete zero-partition set, although PruneFileScanPartition returns that sentinel when the connector view is UNAVAILABLE (an unrepresentable connector partition) and the scan then reads EVERY partition. Recording the sentinel's empty map told PartitionCompensator the query reads no partitions at all, which rejects an otherwise eligible MV rewrite; NOT_PRUNED now records the scan-all marker, while a genuinely empty MATERIALIZED selection still stays concrete, because widening that one would union-compensate partitions the query provably never reads. (3) HiveConnectorMetadata's local fallback decoded a rendered partition name into a map keyed by the RENDERED key, but Hive's FileUtils.makePartName lowercases partition keys, so a declared key such as `P-X` never matched the rendered `p-x=1`: every real partition was rejected, and because this prefilter's result becomes the logical selected view, the query returned no rows. Values are now bound POSITIONALLY to the declared partition keys, and a name whose segment count cannot be bound is KEPT instead of dropped (never fewer rows; the typed PartitionPruner re-prunes the survivors, so a superset only costs the lost optimization). The value-lookup null check that positional binding makes unreachable is dropped with it, so an unbound name can no longer silently prune the whole table away. ### Release note None ### Check List (For Author) - Test: Unit Test (BUILD SUCCESS, FE checkstyle 0 violations on fe-core and fe-connector-hive). New QueryPartitionCollectorTest 6 (preloaded view reused with `verify(never())` proving no connector call under the lock, unavailable view -> scan-all, no-preload fallback still enumerates, version-selector reference does not reuse the latest preload, NOT_PRUNED -> scan-all, real empty selection stays empty); StatementContextTest 11 (2 new: the pre-lock pass materializes and records the view; a table without connector pruning is skipped); HiveConnectorMetadataPartitionPruningTest 25 (4 new: positional binding of a lowercased rendered key, escaped declared keys, arity mismatch rejected as uninterpretable, end-to-end local fallback that keeps the matching partition of an uppercase special-char key, and an undecodable name leaving the handle untouched instead of pruning). Regression run of the touched areas also green: HiveConnectorMetadata*Test (all classes), HiveScanBatchModeTest 21, PartitionCompensatorTest 13, LogicalFileScanTest 3. The mixed internal/Hive MV-rewrite plan itself needs a Hive metastore, so it is covered by the external suites (nereids_rules_p0.mv.external_table.part_partition_invalid, mtmv_p0.test_hive_rewrite_mtmv) rather than locally. - Behavior changed: Yes (an unavailable connector partition view is no longer reported as "the query reads no partitions", so MV rewrites that were wrongly rejected become eligible; the hive local fallback no longer prunes a table to zero rows when the rendered partition key differs from the declared spelling) - Does this need documentation: No
…locks by default ### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: The previous commit moved the MV partition collector's enumeration of a deferred (connector-pruning) file scan out of the lock window, but only through the pre-lock preload pass, which is gated on the opt-in `enable_preload_external_metadata` session variable. With that variable at its default (off) the collector still resolved the view itself, i.e. QueryPartitionCollector - which runs from InitMaterializationContextHook.afterRewrite, after StatementContext.lock() - still performed an unbounded connector round-trip plus an O(all partitions) item build while the statement held read locks on every internal table of the same query, blocking their DDL and metadata writers for that whole window. NereidsPlanner.collectAndLockTable now materializes those views unconditionally in the pre-lock phase (the same place the preload rule already runs when enabled), and the collector reuses them, so the default configuration gets the lock scope without any session-variable default being changed. The step is skipped when nothing is locked during planning, and when MV rewrite is disabled because the materialized view then has no consumer at all; it stays limited to tables whose connector can prune from a predicate and to the LATEST reference, which is the only shape the collector reuses. The one cost is that a mixed internal/Hive query which would have been pruned entirely by an HMS filter now also pays one unfiltered listing before the lock - the same listing the collector paid under the lock before, and the same one the pre-deferral code paid at bind time - which is the trade the lock scope is bought with. ### Release note None ### Check List (For Author) - Test: Unit Test (BUILD SUCCESS; FE checkstyle 0 violations including fe-core). New QueryPartitionCollectorTest case `defaultConfigurationMaterializesTheViewBeforeTheLock` runs the real pre-lock step and then the collector on the DEFAULT session variables, asserts the preload switch is still off, and proves with `verify(never())` that no connector call happens after the lock boundary; QueryPartitionCollectorTest 7 and StatementContextTest 14 pass, the latter with 3 new cases pinning the step's gating (skipped without an internal plan-time read lock, skipped when MV rewrite is disabled, and both without a connector call). Regression run of the touched areas also green: 568 fe-core tests in the PluginDriven*/MV/planner classes (incl. PluginDrivenMvccExternalTableTest 82, PartitionCompensatorTest 13, LogicalFileScanTest 3, NereidsPlannerTest 2) plus the fe-connector-hive classes (HiveConnectorMetadata*Test, HiveScanBatchModeTest 21). - Behavior changed: Yes (a mixed internal-table/Hive query now resolves the Hive partition view before taking the internal read locks instead of inside them; no session variable default is modified) - Does this need documentation: No
…ble reference ### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Fixes the findings of a local pipeline-style review of the previous head (two Major, two Minor). (1) The MV partition compensator and the physical scan read the base table's partition set from two independent connector enumerations: the collector records the names it read (now from the pre-lock warmup), while PluginDrivenScanNode materializes its own, later, view in doFinalize. Because the compensation union branch is restricted to exactly the recorded names (StructInfo.addFilterOnTableScan builds an OR over those partition items and the re-pruned copy keeps them), a base partition added between the two reads is read by neither branch and its rows silently disappear from a rewritten query; the two reads coincide only when the connector's partition-view cache happens to serve both, so a REFRESH TABLE, the cache TTL or a catalog with meta.cache.hive.partition_view.enable=false opens the window. The view is now resolved once per statement and table reference: StatementContext.resolveScanPartitionView returns the recorded view to every consumer - the MV partition collector, the PruneFileScanPartition full-view fallback and the scan's own deferred materialization - and records the first enumeration, so the compensation decision and the data read describe the same partition set; a reference carrying a version selector is deliberately neither served nor recorded, because the record is per table and holds the LATEST generation only. (2) The pre-lock warmup added by the previous commit was gated on enable_materialized_view_rewrite alone, but InitConsistentMaterializationContextHook (the DML hook, registered from enable_dml_materialized_view_rewrite) extends InitMaterializationContextHook and therefore also runs the collector: with query-level MV rewrite off and DML rewrite on, an INSERT INTO <olap> SELECT ... FROM <hive> still enumerated the view under the statement's internal table read locks, and the opposite polarity warmed a view nothing consumed. The gate is now the OR of the two switches. (3) resolveUnknownTotalPartitionNum left totalPartitionNum at -1 when the connector view was UNAVAILABLE, so every later render of the same node (toString, getPlanTreeExplainStr, Profile.updateSummary) rebuilt the whole O(all partitions) view for the same answer; the attempt is now memoized. (4) The HMS filtered-probe size was the compile-time default batch size (5000 + 1): the metastore validates the REQUESTED max_parts against metastore.limit.partition.request, so a metastore hardened below 5001 rejected every filter call and lost the feature entirely, and the per-catalog hive.hms_partitions_batch_size_per_rpc was ignored. The threshold now comes from the configured batch size (clamped below Short.MAX_VALUE, since the probe is threshold + 1 and the Thrift field is a short), which also gives an operator a knob to match a hardened metastore. Also closes the latent PREPARE/EXECUTE hole the review noted: resetMvccSnapshots() now drops the recorded views, so a reused statement cannot pin the previous execution's partition set. ### Release note None ### Check List (For Author) - Test: Unit Test (BUILD SUCCESS; FE checkstyle 0 violations for fe-core and fe-connector-hms). StatementContextTest 18 (5 new: the DML-hook gate polarity, both-switches-off skip, one materialization per table reference, the unavailable view staying unavailable, and a versioned reference neither served nor recorded), QueryPartitionCollectorTest 7 (the collector's own enumeration is now asserted to be recorded for the scan), ThriftHmsClientMaxPartsTest 6 (threshold follows the configured batch size), plus the regression set of the touched areas: PluginDrivenScanNode*Test (all classes), PartitionCompensatorTest 13, NereidsPlannerTest 2, HiveConnectorMetadataPartitionPruningTest 25, PluginDrivenExternalTablePartitionTest 9. The mixed internal/Hive MTMV scenario itself needs a metastore, so it is covered by the external suites (nereids_rules_p0.mv.external_table.part_partition_invalid, mtmv_p0.test_hive_rewrite_mtmv). - Behavior changed: Yes (an unfiltered connector-pruning scan now reads the partition set its statement's MV compensation decision was based on instead of a second, later enumeration; the warmup gate covers the DML MV rewrite configuration; the HMS filtered probe follows the per-catalog batch size) - Does this need documentation: No
…d EXPLAIN resolutions ### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Two review Nits from the previous round. (1) `filteredPartitionThreshold` guarded its lower bound with `Math.max(1, ...)`, but `HmsClientConfig` already rejects a non-positive batch size in its constructor, so that half was unreachable while the load-bearing upper bound (`Short.MAX_VALUE - 1`, because the probe is threshold + 1 and the Thrift max_parts field is a short) had no test; the clamp is now a package-private static that takes the batch size, and `ThriftHmsClientMaxPartsTest` pins both the configured value and the cap. (2) `resolveUnknownTotalPartitionNum` set its memo flag before the connector call, so an attempt that threw was recorded as resolved: the first render propagated the failure while every later render of the same node silently printed `partition=N/?`. Only a completed resolution is memoized now, so the UNAVAILABLE case (which does not throw) is still asked once, and a genuine failure keeps failing. ### Release note None ### Check List (For Author) - Test: Unit Test (BUILD SUCCESS; FE checkstyle 0 violations for fe-core and fe-connector-hms). ThriftHmsClientMaxPartsTest 7 (new `testFilteredProbeFollowsTheBatchSizeAndStaysInShortRange`: the configured value is returned unchanged and both the Short.MAX_VALUE and 1<<20 inputs clamp to Short.MAX_VALUE - 1), HmsPartitionBatchExecutorTest, StatementContextTest 18, QueryPartitionCollectorTest 7 and every PluginDrivenScanNode*Test class green. - Behavior changed: Yes (a failed EXPLAIN-total resolution is no longer cached as resolved, so the failure is reported on every render instead of degrading to `?`; the filtered-probe threshold is unchanged for every configurable batch size) - Does this need documentation: No
…invariant ### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Closes the four findings of the previous review round, one of them a regression that round's own Nit fix introduced. (1) `resolveUnknownTotalPartitionNum` memoized only COMPLETED resolutions, so an attempt whose connector call threw was retried by every later render; the repeat caller is the query-profile path, which swallows the throw (`StmtExecutor.updateProfile` catches Throwable with a WARN), so a failing lookup rebuilt the whole partition view on every profile update, logged the WARNs again and kept aborting the update, leaving the profile unpublished. The attempt is now memoized together with its outcome: the throwable is stored and rethrown, so the connector is asked at most once per node and the failure is stable. (2) The filtered-probe test asserted the current constant instead of the invariant; it now asserts `threshold + 1 <= Short.MAX_VALUE` over a set of batch sizes (including the identity boundary `Short.MAX_VALUE - 1`) plus the unchanged configured value, so adjusting the cap stays valid while the probe still fits. (3) That memo state machine - the actual behaviour change - had no test; `PluginDrivenScanNodeExplainTotalMemoTest` now drives the resolver through a real scan node and pins all three shapes: an unavailable view asked once and leaving the total unknown, an available view asked once and resolving the count, and a failing lookup asked once with the stored failure rethrown. (4) The cap rationale named the wrong narrowing mechanism: the production path saturates an oversized `max_parts` to `Short.MAX_VALUE` (`HiveMetaStoreClient.shrinkMaxtoShort`), which is what would make a truncated page look complete, while a plain `(short)` cast would go negative and fail loud. ### Release note None ### Check List (For Author) - Test: Unit Test (BUILD SUCCESS; FE checkstyle 0 violations for fe-core and fe-connector-hms). New PluginDrivenScanNodeExplainTotalMemoTest 3 (the three resolver shapes above), ThriftHmsClientMaxPartsTest 7, StatementContextTest 18, QueryPartitionCollectorTest 7 and every PluginDrivenScanNode*Test class green. - Behavior changed: Yes (a failed EXPLAIN-total lookup is now remembered and rethrown instead of re-querying the connector on every render, so the query profile can be published after such a failure) - Does this need documentation: No
…y claim ### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Closes the two test-fidelity findings of the previous review round. (1) The failure case stubbed the lookup with a single-instance `thenThrow`, and Mockito rethrows that same object on every call, so the `assertSame(first, second)` assertion held even for an implementation that re-queries the connector - only the `verify(times(1))` was load-bearing. The stub now answers with a FRESH exception per invocation, so the identity assertion proves the stored failure was rethrown instead of the lookup being repeated. (2) The tests drove the private resolver directly, so removing its call from `getNodeExplainString` would have left the suite green: no test rendered a connector-backed node with an unknown total. A render-level case now builds a node the renderer can run without I/O (the connector-filtered shape: known selected count, unknown total, available view) and asserts the emitted `partition=N/M` line, proving the completed total actually reaches EXPLAIN. ### Release note None ### Check List (For Author) - Test: Unit Test (BUILD SUCCESS; FE checkstyle 0 violations for fe-core). PluginDrivenScanNodeExplainTotalMemoTest 4 - the unavailable / available / failing shapes plus the new render-level case asserting `partition=1/1` and a single connector lookup across two renders. - Behavior changed: No (test-only change) - Does this need documentation: No
…laim ### What problem does this PR solve? Issue Number: Related PR: apache#67725 Problem Summary: Closes the two test-strength findings of the previous review round. (1) The render fixture held ONE partition in the unfiltered view and seeded a selected count of one, so the asserted `partition=1/1` also came out of an implementation that completes the total from the selection size rather than from the connector's unfiltered view - the very distinction the EXPLAIN-total work exists for. The view now holds two partitions while the selection still reports one, and the assertion is `partition=1/2`. (2) The re-render assertion repeated the first one verbatim and could never fail independently; the re-render is kept (it is what makes the "one connector lookup per node" assertion meaningful) with the duplicate assertion replaced by a comment naming that purpose. ### Release note None ### Check List (For Author) - Test: Unit Test (BUILD SUCCESS; FE checkstyle 0 violations for fe-core). PluginDrivenScanNodeExplainTotalMemoTest 4 - the unavailable / available / failing resolver shapes plus the render-level case, now asserting the total comes from the unfiltered view (`partition=1/2` with a one-partition selection) across two renders with a single connector lookup. - Behavior changed: No (test-only change) - Does this need documentation: No
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
3 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?
Issue Number: close #67724
Related PR: #67739
Problem Summary: Hive partition pruning previously built the generic partition view by enumerating every HMS partition name before applying selective predicates. For highly partitioned tables, planning therefore depended on a full metastore listing. This change introduces a connector-filtered partition view for plain Hive tables, translates safe equality and IN predicates into the connector filter grammar during logical partition pruning, and builds the selected-partition map from HMS
get_partitions_by_filterresults. The existing full-list local pruning path remains the fallback when the HMS API or filter dialect is unavailable. A separate deferred partition state preserves batch split generation for no-filter full scans.Known follow-up: logical pruning and physical handle preparation can still repeat connector-native partition filtering. The explicit result/handle propagation work is tracked in #67739.
Release note
Improve planning latency for selective Hive partition queries when HMS supports
get_partitions_by_filter.Check List (For Author)
HiveConnectorMetadataPartitionPruningTest(16 tests)PluginDrivenExternalTablePartitionTest,PluginDrivenScanNodeBatchModeTest,PluginDrivenScanNodePartitionPruningTest, andPluginDrivenScanNodePartitionCountTest(41 tests)DISABLE_BUILD_UI=ON ./build.sh --fegit diff --check