Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22105 )
Change subject: IMPALA-13517: Support overloaded || operator ...................................................................... Patch Set 13: (5 comments) http://gerrit.cloudera.org:8080/#/c/22105/10/java/calcite-planner/src/main/codegen/templates/Parser.jj File java/calcite-planner/src/main/codegen/templates/Parser.jj: http://gerrit.cloudera.org:8080/#/c/22105/10/java/calcite-planner/src/main/codegen/templates/Parser.jj@7744 PS10, Line 7744: <PLUS> { return ImpalaCustomOperatorTable.PLUS; } : | <MINUS> { return ImpalaCustomOperatorTable.MINUS; } : | <STAR> { return ImpalaCustomOperatorTable.MULTIPLY; } : | <SLASH> { return ImpalaCustomOperatorTable.DIV > just out of curiosity, in ImpalaCustomOperatorTable, you redefine following I think this works right now because it winds up changing the operator later on. But it makes sense to be consistent. Changed it. http://gerrit.cloudera.org:8080/#/c/22105/10/java/calcite-planner/src/main/codegen/templates/Parser.jj@7754 PS10, Line 7754: ImpalaCustomOperatorTable.CONCA > in order to use CONCAT_OR you defined in ImpalaCustomOperatorTable, maybe h Also here, makes sense to be consistent. Changed it. http://gerrit.cloudera.org:8080/#/c/22105/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaConcatOrOperator.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaConcatOrOperator.java: http://gerrit.cloudera.org:8080/#/c/22105/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaConcatOrOperator.java@45 PS10, Line 45: public class ImpalaConcatOrOperator extends SqlBinaryOperator { > IMPALA-13833, maybe div's solution is similar to this? IIRC, I think "div" is even simpler? I think I fixed this in a local branch. I'll have to check. But the Jira is filed: IMPALA-13833 http://gerrit.cloudera.org:8080/#/c/22105/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaConcatOrOperator.java@67 PS10, Line 67: return SqlOperandCountRanges.of(2); > Nit: Should this be between(2, 2)? The max value is inclusive. Changed to "of(<x>)" http://gerrit.cloudera.org:8080/#/c/22105/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaCustomOperatorTable.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaCustomOperatorTable.java: http://gerrit.cloudera.org:8080/#/c/22105/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaCustomOperatorTable.java@225 PS10, Line 225: CONCAT_OR > hi, where will use 'CONCAT_OR' It's used now in the Parser.jj file. But regardless, it needed to be here because it is picked up in the parent ReflectiveSqlOperatorTable class which examines variables in its class through reflection. -- To view, visit http://gerrit.cloudera.org:8080/22105 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iabaf02e84b769db1419bd96e1d1b30b8f83d56f5 Gerrit-Change-Number: 22105 Gerrit-PatchSet: 13 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Anonymous Coward (816) Gerrit-Reviewer: Fang-Yu Rao <[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: Thu, 01 May 2025 19:32:45 +0000 Gerrit-HasComments: Yes
