Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24074 )
Change subject: IMPALA-14482: Calcite planner: order by "random()" fixed ...................................................................... Patch Set 3: (4 comments) I put this in one of the comments, but thanks for getting me to rethink this! Yes, there was a much easier way to fix this without overriding the rule. http://gerrit.cloudera.org:8080/#/c/24074/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaSortRemoveConstantKeysRule.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaSortRemoveConstantKeysRule.java: http://gerrit.cloudera.org:8080/#/c/24074/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaSortRemoveConstantKeysRule.java@62 PS3, Line 62: final RexBuilder rexBuilder = sort.getCluster().getRexBuilder(); > nit: this rexBuilder is unused. See comment below http://gerrit.cloudera.org:8080/#/c/24074/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaSortRemoveConstantKeysRule.java@83 PS3, Line 83: sort.copy(sort.getTraitSet(), input, RelCollations.of(collationsList)); > This has changed in Calcite 1.41.0. Should we pick up https://github.com/ap See comment below http://gerrit.cloudera.org:8080/#/c/24074/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaSortRemoveConstantKeysRule.java@83 PS3, Line 83: sort.copy(sort.getTraitSet(), input, RelCollations.of(collationsList)); > Specific bug fixed was https://issues.apache.org/jira/browse/CALCITE-6460. See comment below http://gerrit.cloudera.org:8080/#/c/24074/3/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaSortRemoveConstantKeysRule.java@95 PS3, Line 95: if (!predicates.constantMap.containsKey(inputRef)) { > Is there a way we could make sure NON_DETERMINISTIC_FNS don't get put in th Heh, yeah, good comment, you got me thinking about it again. As you can see with the latest commit, the operator itself allows you to override the isDeterministic() method. Once I did that, the change was picked up in the framework and the function was no longer treated as constant. So the change is a lot simpler now :) -- To view, visit http://gerrit.cloudera.org:8080/24074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ebf3aa11030d98b447d08d1364f5f65eb2ca661 Gerrit-Change-Number: 24074 Gerrit-PatchSet: 3 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[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: Fri, 13 Mar 2026 15:00:56 +0000 Gerrit-HasComments: Yes
