Steve Carlin 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: (6 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 w I added the basic ones, but not all. I did not include the distinct ones nor the "nor equals" Eventually everything should be tested once we have all the Impala tests run through the Calcite planner so I think this is ok for now? Thoughts? 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? Correct. Do you think I should make more general changes namewise to express that? 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. Done http://gerrit.cloudera.org:8080/#/c/21237/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSortRel.java@153 PS9, Line 153: private NodeWithExprs getChildPlanNode(ParentPlanRelContext context) throws ImpalaException { > line too long (95 > 90) Done 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 Done 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- I mentioned this for an above comment...there were so few of these that I went ahead and did them. In general, I figure that once all the Impala tests are enabled, it will cover all variations so I'm not too worried about being overly comprehensive. -- 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-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Wed, 10 Jul 2024 16:45:54 +0000 Gerrit-HasComments: Yes
