[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/24095 )
Change subject: IMPALA-7618: Accept reversed comparators (> / >=) in range partition bounds ...................................................................... Patch Set 5: (14 comments) Thanks for the review! Addressed all comments in the latest patch set. http://gerrit.cloudera.org:8080/#/c/24095/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24095/1//COMMIT_MSG@21 PS1, Line 21: > nit: Convention is: Done. Updated the trailer to "Generated-by: Claude Opus 4.8". http://gerrit.cloudera.org:8080/#/c/24095/2/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/24095/2/fe/src/main/cup/sql-parser.cup@2083 PS2, Line 2083: // The bound that a > nit: since it is now not necesserily a lower-bound, its name could be 'expr Done. Renamed to expr_cmp_range_val, since the bound before VALUES can be either a lower or an upper bound depending on the comparator. http://gerrit.cloudera.org:8080/#/c/24095/2/fe/src/main/cup/sql-parser.cup@2107 PS2, Line 2107: {: RESULT = null; > nit: could be "cmp_expr_range_val" Done. Renamed to cmp_expr_range_val. http://gerrit.cloudera.org:8080/#/c/24095/1/fe/src/main/java/org/apache/impala/analysis/RangeBound.java File fe/src/main/java/org/apache/impala/analysis/RangeBound.java: http://gerrit.cloudera.org:8080/#/c/24095/1/fe/src/main/java/org/apache/impala/analysis/RangeBound.java@36 PS1, Line 36: > I think the could would become cleaner at a lot of places if you'd introduc Done. Introduced a single Comparator enum (LESS_THAN, LESS_EQUAL, GREATER_THAN, GREATER_EQUAL), which is now used across the grammar and RangePartition. http://gerrit.cloudera.org:8080/#/c/24095/2/fe/src/main/java/org/apache/impala/analysis/RangeBound.java File fe/src/main/java/org/apache/impala/analysis/RangeBound.java: http://gerrit.cloudera.org:8080/#/c/24095/2/fe/src/main/java/org/apache/impala/analysis/RangeBound.java@41 PS2, Line 41: } : } : > This could be removed Done, removed. http://gerrit.cloudera.org:8080/#/c/24095/2/fe/src/main/java/org/apache/impala/analysis/RangeBound.java@45 PS2, Line 45: LOWER_BOUND, > We could add: Done, added the BoundType enum and boundType() as suggested. Since the grammar now normalizes the comparator to the "bound <cmp> VALUES" form, boundType() is a pure function of the comparator ('<'/'<=' -> LOWER_BOUND, '>'/'>=' -> UPPER_BOUND). http://gerrit.cloudera.org:8080/#/c/24095/2/fe/src/main/java/org/apache/impala/analysis/RangeBound.java@57 PS2, Line 57: public List<Expr> getValues() { return values_; } > This could be removed Done, removed. http://gerrit.cloudera.org:8080/#/c/24095/2/fe/src/main/java/org/apache/impala/analysis/RangePartition.java File fe/src/main/java/org/apache/impala/analysis/RangePartition.java: http://gerrit.cloudera.org:8080/#/c/24095/2/fe/src/main/java/org/apache/impala/analysis/RangePartition.java@103 PS2, Line 103: > nit: could be 'first' Done, renamed to first. http://gerrit.cloudera.org:8080/#/c/24095/2/fe/src/main/java/org/apache/impala/analysis/RangePartition.java@104 PS2, Line 104: angeP > nit: 'second' Done, renamed to second. http://gerrit.cloudera.org:8080/#/c/24095/2/fe/src/main/java/org/apache/impala/analysis/RangePartition.java@107 PS2, Line 107: RangeBound upper = null; : if (first != null) { : if (first.boundType() == RangeBound.BoundType.LOWER_BOUND) { : lower = first; : } else { : upper = first; : } : } > We could have: Done, applied your suggested structure. createFromRangeWithNormalization() now assigns each bound to lower/upper via boundType() and throws an AnalysisException when both bounds constrain the same side (e.g. '4 > VALUES < 2'). http://gerrit.cloudera.org:8080/#/c/24095/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java: http://gerrit.cloudera.org:8080/#/c/24095/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@195 PS1, Line 195: (partition values >= 1, partition 4 > values > Is it possible to add test for Done, added a test for "partition 4 > values >= 1" (both bounds reversed). http://gerrit.cloudera.org:8080/#/c/24095/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java: http://gerrit.cloudera.org:8080/#/c/24095/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@207 PS2, Line 207: "stored as kudu", isExternalPurgeTbl); > Could you please add some tests for errors? Added error tests for "4 > values < 2" (Multiple upper bounds...) and "4 < values > 1" (Multiple lower bounds...), which the new same-side-bound validation catches. The other two ("values > 4, values > 5" and "values < 2, values < 1") are overlapping ranges across separate partitions. Those are rejected by Kudu at table-creation time, not during analysis, so they can't be covered in AnalyzeKuduDDLTest. Happy to add them as an end-to-end Kudu test if you prefer. http://gerrit.cloudera.org:8080/#/c/24095/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java: http://gerrit.cloudera.org:8080/#/c/24095/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@210 PS3, Line 210: AnalysisError("create table tab (a int, b int, c int, d int, " + > line too long (92 > 90) Done. Split the string literal so the line is within 90 columns. http://gerrit.cloudera.org:8080/#/c/24095/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@214 PS3, Line 214: "Multiple upper bounds specified for a range partition.", isExternalPurgeTbl); > line too long (92 > 90) Done. Split the string literal so the line is within 90 columns. -- To view, visit http://gerrit.cloudera.org:8080/24095 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia12668537bfc72cc9399a60f68ca7608422b35c2 Gerrit-Change-Number: 24095 Gerrit-PatchSet: 5 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 09 Jul 2026 10:19:47 +0000 Gerrit-HasComments: Yes
