ji chen 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 4: (12 comments) 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 No currently. 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: P > nit: extra space compared to the comments above Done http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/PaimonScanNode.java@127 PS3, Line 127: protected void applyPaimo > nit: fits line above Done 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@58 PS3, Line 58: import java.sql.Timestamp; : import java.time.LocalDateTime; > Can you create FE test to test all public methods in ImpalaExprConverter? Done http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@120 PS3, Line 120: } else if (filter instanceof BinaryPredicate) { : BinaryPredicate binaryPredicate = (BinaryPredicate) filter; : SlotRef attribute; : LiteralExpr literal; : if (binaryPredicate.getChild(1) instanceof LiteralExpr) { : literal = (LiteralExpr) binaryPredicate.getChild(1); : attribute = extractSlotRef(binaryPredicate.getChild(0)); : } else { : literal = (LiteralExpr) binaryPredicate.getChild(0); : attribute = extractSlotRef(binaryPredicate.getChild(1)); : } : String field = attribute.getResolvedPath().destColumn().getName(); : int index = fieldIndex(field); : Object literal_value = convertLiteral(index, literal); : switch (binaryPredicate.getOp()) { : case EQ: : return > Can we have tests with all thes predicates, and using NULL values as well? Done 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 Done http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/PaimonPartitionPredicateVisitor.java@46 PS3, Line 46: * This function will check whether the given predicate is partition-only predicate : * expression, if yes, the partition predicate will not be added to the postscan : * expressions to reduce filter cost. It assumed that all partition-only predicate : * can accurately filter out partitions by the given partition-only predicate. : * */ : public Boolean > So all child predicate needs to refer partition keys. I think it'd good to Done 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 o Done 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 Done 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 a Done http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/PredicateExtractor.java@68 PS3, Line 68: > nit: This method doesn't return any objects. Done http://gerrit.cloudera.org:8080/#/c/23793/3/fe/src/main/java/org/apache/impala/planner/paimon/PredicateExtractor.java@88 PS3, Line 88: partitionExprs_.add(expr); : } else { : postScanExprs_.add(expr); : } : } : } : } : : 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 pushedPredica Done -- 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: 4 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: Sun, 28 Dec 2025 15:16:03 +0000 Gerrit-HasComments: Yes
