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 10: (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") > I added the basic ones, but not all. I did not include the distinct ones n That seems fine. I realize this was primarily only needed for sorting, and wasn't sure whether we'd also need "lt" to implement ASC/DESC. 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@146 PS9, Line 146: // If there is a filter condition, a SelectNode will get added on top > Done Ack 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 > Done Ack 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 > I mentioned this for an above comment...there were so few of these that I w Thanks. I agree there's a balance to strike between testing for Calcite specifically, and working towards getting the full test suite working. Easy coverage is good though, and I'm not sure how thorough some of our existing testing is. http://gerrit.cloudera.org:8080/#/c/21237/10/testdata/workloads/functional-query/queries/QueryTest/calcite.test File testdata/workloads/functional-query/queries/QueryTest/calcite.test: http://gerrit.cloudera.org:8080/#/c/21237/10/testdata/workloads/functional-query/queries/QueryTest/calcite.test@198 PS10, Line 198: 4,0 Is the sort order if id deterministic? It's not specified in the query. Not sure whether the test can assume that or needs to be explicit. I think this may be ok to assume for NUM_NODES=1, but not in a distributed plan. -- 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: 10 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 18:14:41 +0000 Gerrit-HasComments: Yes
