Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21237 )
Change subject: IMPALA-12954: Implement Sorting capability for Calcite planner ...................................................................... Patch Set 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java: http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java@51 PS9, Line 51: .put(SqlKind.GREATER_THAN, "gt") Could we add a full set of binary comparison functions here and test them with this patch? http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java: http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@103 PS9, Line 103: // If there's limit without order-by, we don't need to generate So Calcite's Sort node covers both limit and order-by? http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@146 PS9, Line 146: return context.filterCondition_ != null Another use for wrapInSelectNodeIfNeeded when you rebase. http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@177 PS9, Line 177: * do not necessarily line up with the indexes. So we need to walk through the nit: extra spaces http://gerrit.cloudera.org:8080/#/c/21237/9/testdata/workloads/functional-query/queries/QueryTest/calcite.test File testdata/workloads/functional-query/queries/QueryTest/calcite.test: http://gerrit.cloudera.org:8080/#/c/21237/9/testdata/workloads/functional-query/queries/QueryTest/calcite.test@195 PS9, Line 195: # sort test Can we test all the variations in https://impala.apache.org/docs/build/asf-site-html/topics/impala_order_by.html? Ascend/descend, nulls, order by column ordinal: ORDER BY col_ref [, col_ref ...] [ASC | DESC] [NULLS FIRST | NULLS LAST] -- To view, visit http://gerrit.cloudera.org:8080/21237 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I747e107ed996862ef348f829deee47f0c0fc78d5 Gerrit-Change-Number: 21237 Gerrit-PatchSet: 9 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Tue, 09 Jul 2024 18:27:18 +0000 Gerrit-HasComments: Yes
