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 6: (18 comments) http://gerrit.cloudera.org:8080/#/c/23793/4/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/23793/4/common/thrift/ImpalaService.thrift@1077 PS4, Line 1077: : // The summary of a DML statement. : struct TDmlResult { > I still think this query option is not necessary, and Paimon scanner should sure, will remove the query option 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: // push down only applies to paimon data table. > nit: I don't think we need these trivial comments here Done 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 Done http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/PaimonScanNode.java@201 PS4, Line 201: > Do we need to check this? predicateExtractor.getPushedPredicates() should a thanks, you are right.will remove http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/PaimonScanNode.java@231 PS4, Line 231: cations(location > Instead of hijacking 'file_metadata', we should add another member to TScan Done 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? Done http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@118 PS4, Line 118: onditi > At this point it would be more informational to throw an error like "Negati Done http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@124 PS4, Line 124: case EQ: : return literal_value != null ? builder.equal(index, literal_value) : : builder.isNull(index); : case NE: : return literal_value != null ? builder.notEqual(index, literal_value) : : builder.isNotNull(index); : > IIRC we normalize predicates to always have literals on the right side, but Done http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@131 PS4, Line 131: case GE: return builder.greaterOrEqual(index, literal_value); : case LT: return builder.less > This repeats multiple times, could be moved to a helper method. Probably ex Done http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@192 PS4, Line 192: upportedOperationException( > lenth is greater than 1, and indexOf("%") returned the index of the last ch Done http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@193 PS4, Line 193: is unsup > typo: candidate Done http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@207 PS4, Line 207: > nit: Supported Done http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/ImpalaExprConverter.java@274 PS4, Line 274: > Can't you just use Expr.unwrapSlotRef()? Done 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 Paimon partition design is similar to hive, it does't have partition specs like iceberg. so for partition level predicates, I think it it enough to evaluate only in java paimon scanner. Impala c++ scanner is not necessary, please let me know If I'm wrong,thanks. http://gerrit.cloudera.org:8080/#/c/23793/4/fe/src/main/java/org/apache/impala/planner/paimon/PredicateExtractor.java@96 PS4, Line 96: : : : : : : > They are now unused, right? No,they are not used, will remove. 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. Done http://gerrit.cloudera.org:8080/#/c/23793/4/tests/query_test/test_paimon.py File tests/query_test/test_paimon.py: http://gerrit.cloudera.org:8080/#/c/23793/4/tests/query_test/test_paimon.py@60 PS4, Line 60: test_paimon_pre > Spell this out: test_paimon_predicate_push_down Done http://gerrit.cloudera.org:8080/#/c/23793/4/tests/query_test/test_paimon.py@61 PS4, Line 61: paimon-predicate-p > Spell this out: paimon-predicate-push-down 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: 6 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, 25 Jan 2026 14:42:58 +0000 Gerrit-HasComments: Yes
