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 4: (19 comments) http://gerrit.cloudera.org:8080/#/c/23793/4/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/4/fe/src/main/java/org/apache/impala/planner/PaimonScanNode.java@118 PS4, Line 118: // check query options for pushdown enabled settings. nit: I don't think we need these trivial comments here http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/PaimonScanNode.java@121 PS4, Line 121: { nit: for single-line if statements we don't need the curly brackets. Should be the same as in L119. This applies to L129 as well. http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/PaimonScanNode.java@128 PS4, Line 128: // check query options for pushdown enabled settings. nit: the code if self-explanatory enough http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/PaimonScanNode.java@201 PS4, Line 201: canApplyPaimonPredicatePushdown(analyzer) Do we need to check this? predicateExtractor.getPushedPredicates() should always be empty wan we cannot apply predicates. http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/PaimonScanNode.java@231 PS4, Line 231: setFile_metadata Instead of hijacking 'file_metadata', we should add another member to TScanRange and ScanRangePB. http://gerrit.cloudera.org:8080/#/c/23793/4/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/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@74 PS4, Line 74: "=" Should we add "!=" as well? http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@118 PS4, Line 118: break; At this point it would be more informational to throw an error like "Negation of predicate is not possible: ...", than falling back to the generic filter is unsupported error. http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@124 PS4, Line 124: 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)); : } IIRC we normalize predicates to always have literals on the right side, but you can double-check. http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@131 PS4, Line 131: String field = attribute.getResolvedPath().destColumn().getName(); : int index = fieldIndex(field); This repeats multiple times, could be moved to a helper method. Probably extractSlotRef() can be moved into that helper as well. http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@192 PS4, Line 192: literal_value.charAt(0) != '%' lenth is greater than 1, and indexOf("%") returned the index of the last character, so first char should never be '%'. http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@193 PS4, Line 193: cadidate typo: candidate http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@194 PS4, Line 194: if (!cadidate.contains("%")) { indexOf("%") returned the last character at L191, so this should be always true. http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@199 PS4, Line 199: if (!candidate.contains("%")) { lastIndexOf() returned 0 above, so this should be always true. http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@207 PS4, Line 207: Support nit: Supported http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@274 PS4, Line 274: extractSlotRef Can't you just use Expr.unwrapSlotRef()? http://gerrit.cloudera.org:8080/#/c/23793/4/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/4/fe/src/main/java/org/apache/impala/planner/paimon/PaimonPartitionPredicateVisitor.java@54 PS4, Line 54: { nit: curly-brackets not needed for single-line if stmts http://gerrit.cloudera.org:8080/#/c/23793/4/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/4/fe/src/main/java/org/apache/impala/planner/paimon/PredicateExtractor.java@90 PS4, Line 90: postScanExprs_.add(expr); Isn't it enough to add 'expr' to pushedExprs? Don't we evaluate these twice? Once by the java Paimon scanner, and once by Impala's C++ scanner? http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/PredicateExtractor.java@96 PS4, Line 96: 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()); : } They are now unused, right? http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/test/java/org/apache/impala/planner/paimon/PaimonImpalaExprConverterTest.java File fe/src/test/java/org/apache/impala/planner/paimon/PaimonImpalaExprConverterTest.java: http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/test/java/org/apache/impala/planner/paimon/PaimonImpalaExprConverterTest.java@107 PS4, Line 107: %s < 1 You could add tests where literal is on the left side. -- 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: Thu, 08 Jan 2026 14:56:14 +0000 Gerrit-HasComments: Yes
