morrySnow commented on code in PR #65919:
URL: https://github.com/apache/doris/pull/65919#discussion_r3635451677
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/RuntimeFilterTranslator.java:
##########
@@ -388,6 +381,22 @@ private org.apache.doris.planner.RuntimeFilter
finalize(org.apache.doris.planner
return origFilter;
}
+ private void
setPartitionPruningMetadata(org.apache.doris.planner.RuntimeFilter
runtimeFilter,
+ ScanNode scanNode,
RuntimeFilterPartitionPruneClassifier.Classification classification) {
+ if (classification.canPrunePartitions()) {
+ Preconditions.checkState(scanNode instanceof OlapScanNode,
+ "partition-pruning runtime filter target must be an
OlapScanNode");
+ runtimeFilter.markTargetCanPrunePartitions(scanNode.getId());
+ ConnectContext rfPruneCtx = ConnectContext.get();
+ if (rfPruneCtx != null
+ &&
rfPruneCtx.getSessionVariable().isEnableRuntimeFilterPartitionPrune()) {
+ ((OlapScanNode)
scanNode).snapshotPartitionBoundariesForRuntimeFilter();
+ }
+ }
Review Comment:
**Suggestion:** Consider guarding `markTargetCanPrunePartitions()` by the
same session variable check, so that `hasRfDrivingPartitionPruning()` can never
return `true` when no snapshot has been taken. Currently, if
`isEnableRuntimeFilterPartitionPrune` changes from `false` (planning) to `true`
(serialization), the `Preconditions.checkNotNull` in
`setPartitionBoundariesForRuntimeFilter()` will fail.
```suggestion
if (classification.canPrunePartitions()) {
// Only mark and snapshot when the session variable is enabled,
// so that hasRfDrivingPartitionPruning() and the snapshot stay
in sync.
ConnectContext rfPruneCtx = ConnectContext.get();
if (rfPruneCtx != null
&&
rfPruneCtx.getSessionVariable().isEnableRuntimeFilterPartitionPrune()) {
Preconditions.checkState(scanNode instanceof OlapScanNode,
"partition-pruning runtime filter target must be an
OlapScanNode");
runtimeFilter.markTargetCanPrunePartitions(scanNode.getId());
((OlapScanNode)
scanNode).snapshotPartitionBoundariesForRuntimeFilter();
}
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/planner/OlapScanNode.java:
##########
@@ -1169,6 +1174,14 @@ public String getNodeExplainString(String prefix,
TExplainLevel detailLevel) {
return output.toString();
}
+ @VisibleForTesting
+ String getSelectedPartitionNamesForExplain() {
+ return getSelectedPartitionIds().stream().sorted()
Review Comment:
**Nit:** Consider using `%d` instead of `%s` for the partition ID since it's
a `Long` (numeric type). The current `%s` works via auto-boxing `toString()`,
but `%d` is more idiomatic and consistent with other Preconditions usages in
the codebase.
```suggestion
.map(id ->
Preconditions.checkNotNull(selectedPartitionNames.get(id),
"missing snapshotted name for selected partition
%d", id))
```
--
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]
