Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/23793 )
Change subject: IMPALA-14092 Part3: Enable predicate pushdown for paimon table. ...................................................................... Patch Set 3: (12 comments) Thanks for working on this! http://gerrit.cloudera.org:8080/#/c/23793/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23793/3//COMMIT_MSG@15 PS3, Line 15: We also introduce a query option: : PAIMON_PREDICATE_PUSHDOWN_ENABLED to turn on/off : the predicate pushdown, by default it is is enabled. Sorry, I might have misunderstood things a bit. What I really meant is that we should have a separate query option for Paimon if we want to make predicate pushdown switchable. But I'm also OK with not having the flag if we are confident enough in the implementation. > keep the option to avoid wrong pushdown result for some corner case. Do we know about any such corner case? http://gerrit.cloudera.org:8080/#/c/23793/3//COMMIT_MSG@18 PS3, Line 18: Do you have any measurements to share here? http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/PaimonScanNode.java File fe/src/main/java/org/apache/impala/planner/PaimonScanNode.java: http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/PaimonScanNode.java@89 PS3, Line 89: nit: extra space compared to the comments above http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/PaimonScanNode.java@127 PS3, Line 127: // in predicateExtractor. nit: fits line above http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java File fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java: http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@120 PS3, Line 120: case EQ: return builder.equal(index, literal_value); : case NE: return builder.notEqual(index, literal_value); : case GT: return builder.greaterThan(index, literal_value); : case GE: return builder.greaterOrEqual(index, literal_value); : case LT: return builder.lessThan(index, literal_value); : case LE: return builder.lessOrEqual(index, literal_value); : : case NULL_MATCHING_EQ: : if (literal_value == null) { : return builder.isNull(index); : } else { : return builder.equal(index, literal_value); : } : case DISTINCT_FROM: : case NOT_DISTINCT: : // can't push down,ignore : break; Can we have tests with all thes predicates, and using NULL values as well? http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/PaimonPartitionPredicateVisitor.java File fe/src/main/java/org/apache/impala/planner/paimon/PaimonPartitionPredicateVisitor.java: http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/PaimonPartitionPredicateVisitor.java@31 PS3, Line 31: */ nit: should be moved to next line http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/PaimonPartitionPredicateVisitor.java@46 PS3, Line 46: for (Predicate child : predicate.children()) { : Boolean matched = (Boolean) child.visit(this); : if (!matched) { return false; } : } : : return true; So all child predicate needs to refer partition keys. I think it'd good to have a comment about this method, explaining the semantics. http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/PredicateExtractor.java File fe/src/main/java/org/apache/impala/planner/paimon/PredicateExtractor.java: http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/PredicateExtractor.java@41 PS3, Line 41: // Predicates/Exprs can be pushed down to paimon table I think it's worth to call out that pushed predicates/exprs are supersets of partition predicates/exprs. http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/PredicateExtractor.java@45 PS3, Line 45: nit: extra space http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/PredicateExtractor.java@57 PS3, Line 57: _ nit: no need for the ending '_' for methods. Applies to the below methods as well. http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/PredicateExtractor.java@68 PS3, Line 68: returns exprs nit: This method doesn't return any objects. http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/PredicateExtractor.java@88 PS3, Line 88: pushable.add(predicatePair); : if (predicate.visit(partitionVisitor)) { : partitionExpr.add(predicatePair); : } else { : postScan.add(expr); : } : } : } : : if (!pushable.isEmpty()) { : this.pushedPredicates_ = getPredicates(pushable); : this.pushedExprs_ = getExpr(pushable); : } : if (!partitionExpr.isEmpty()) { : this.partitionPredicates_ = getPredicates(partitionExpr); : this.partitionExprs_ = getExpr(partitionExpr); : } : if (!postScan.isEmpty()) { this.postScanExprs_ = postScan; } : } : : public static List<Predicate> getPredicates(List<Pair> list) { : return list.stream().map(p -> (Predicate) p.getSecond()).collect(Collectors.toList()); : } : : public static List<Expr> getExpr(List<Pair> list) { : return list.stream().map(p -> (Expr) p.first).collect(Collectors.toList()); : } Wouldn't it be shorter to just add expr/predicate directly to pushedPredicate/Expr, partitionPredicate/Expr, and postScanExprs? -- To view, visit http://gerrit.cloudera.org:8080/23793 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iee07fa35de8381121a20b2976d6424626d705483 Gerrit-Change-Number: 23793 Gerrit-PatchSet: 3 Gerrit-Owner: ji chen <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: ji chen <[email protected]> Gerrit-Comment-Date: Fri, 19 Dec 2025 16:32:35 +0000 Gerrit-HasComments: Yes
