Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 )
Change subject: IMPALA-12935: First pass on Calcite planner functions ...................................................................... Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/21357/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/21357/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java@34 PS4, Line 34: stateana > Nit: spelling? Is this supposed to be "that is always in an analyzed state" Done http://gerrit.cloudera.org:8080/#/c/21357/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java: http://gerrit.cloudera.org:8080/#/c/21357/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java@219 PS5, Line 219: return Objects.hash(func, argTypes.get(0).getPrimitiveType().toString()); > How does this interact with inexact matching of types? The thing that pops I don't think this matters on the hashCode. While hashCode and equals need to work together, I think it's ok to have the hashCode match things that are a little more broad. It could theoretically cause an extra collision in a hash bucket, but if one is doing a HashMap.get(), it's still gonna hit the "equals" method. But I don't think there will really be too many extra collisions either because the first parameter is usually different for every function. If you're asking about how the code will work when the signature doesn't match and there is an implicit conversion? I have that coded and committed in a draft. That will be available shortly. http://gerrit.cloudera.org:8080/#/c/21357/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java: http://gerrit.cloudera.org:8080/#/c/21357/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaProjectRel.java@32 PS5, Line 32: import org.apache.impala.analysis.SlotDescriptor; : import org.apache.impala.analysis.SlotRef; > Nit: These look unused? Done -- To view, visit http://gerrit.cloudera.org:8080/21357 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 Gerrit-Change-Number: 21357 Gerrit-PatchSet: 5 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, 01 May 2024 03:23:39 +0000 Gerrit-HasComments: Yes
