924060929 commented on PR #67725: URL: https://github.com/apache/doris/pull/67725#issuecomment-5600156686
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 `PruneFileScanPartition`, `nameToPartitionItem` is reassigned on the connector-filtered and fallback paths, then captured by the lambda at line 147: ```java .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 `f5633377a1e11420ef6a101e5af2f63e2bb9aac9` fails with: ```text PruneFileScanPartition.java:[147,79] local variables referenced from a lambda expression must be final or effectively final ``` The build reached `fe-core` after the preceding reactor modules compiled, so this is not the generated parser/proto mismatch mentioned in the PR description. Please fix this and rerun the FE compilation plus the two new `fe-core` tests. The 16 `HiveConnectorMetadataPartitionPruningTest` cases passed locally, but the `fe-core` tests could not start because main compilation failed. **The deferred partition state should be represented explicitly.** `NOT_PRUNED` and `DEFERRED_PARTITION_PRUNING` currently have identical field values and are distinguished only by singleton identity via `==`. This creates a hidden invariant across logical rewrites, plan copies and physical translation. Please use an explicit state/enum, or another value-based representation, instead of object identity. Relatedly, `Math.max(nameToPartitionItem.size(), 1)` stores a synthetic value in `totalPartitionNum` to distinguish a genuine prune-to-zero result from an unmaterialized partition universe. That field is also used for `EXPLAIN partition=N/M` and partition accounting, so a filtered table can be reported as `3/3` or `0/1` even when the real table has many more partitions. Please represent unknown total count/materialization state separately rather than encoding control state in a fake partition count. **There is also duplicated HMS work.** The logical pruning path calls `listPartitions(filter)` and obtains filtered `HmsPartitionInfo`, converts it to generic `PartitionItem`, and discards the connector-native metadata. Later `PluginDrivenScanNode.convertPredicate()` invokes Hive `applyFilter()` with the original predicate, which calls `get_partitions_by_filter` again to rebuild the `HiveTableHandle`. Thus one selective query can issue the same HMS filter RPC twice and still retain the full original predicate on BE because partial residual matching is not implemented. 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
