[GitHub] [beam] ibzib commented on a change in pull request #13898: [BEAM-11747] Make BeamCalcRel safe for ZetaSQL

[GitBox <gi...@apache.org>]

ibzib commented on a change in pull request #13898:
URL: https://github.com/apache/beam/pull/13898#discussion_r577146350



##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -81,6 +89,13 @@
   "nullness" // TODO(https://issues.apache.org/jira/browse/BEAM-10402)
 })
 public class ZetaSQLQueryPlanner implements QueryPlanner {
+  public enum Calc {

Review comment:
       Nit: can we rename this to avoid confusion with the actual [Calc](https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/core/Calc.java)?

##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -129,20 +149,61 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
         for (RexNode rexNode : logicalCalc.getProgram().getExprList()) {
           if (rexNode instanceof RexCall) {
             RexCall call = (RexCall) rexNode;
-            if (call.getOperator() instanceof SqlUserDefinedFunction) {
+            final SqlOperator operator = call.getOperator();
+
+            CallImplementor implementor = RexImpTable.INSTANCE.get(operator);
+            if (implementor == null) {
+              // Reject methods with no implimentation

Review comment:
       ```suggestion
                 // Reject methods with no implementation
   ```
   

##########
File path: sdks/java/extensions/sql/zetasql/src/test/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSqlJavaUdfTest.java
##########
@@ -222,13 +224,18 @@ public void testBinaryJavaUdf() {
     String sql =
         String.format(
             "CREATE FUNCTION matches(str STRING, regStr STRING) RETURNS BOOLEAN LANGUAGE java OPTIONS (path='%s'); "
-                + "SELECT matches(\"a\", \"a\"), 'apple'='beta'",
+                + "SELECT matches('a', 'a'), matches('apple', 'beta')",

Review comment:
       - Would the old version of this query succeed now?
   - I assume it is okay to remove the old query because we are getting the same coverage with internal compliance tests?

##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -129,20 +149,61 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
         for (RexNode rexNode : logicalCalc.getProgram().getExprList()) {
           if (rexNode instanceof RexCall) {
             RexCall call = (RexCall) rexNode;
-            if (call.getOperator() instanceof SqlUserDefinedFunction) {
+            final SqlOperator operator = call.getOperator();
+
+            CallImplementor implementor = RexImpTable.INSTANCE.get(operator);
+            if (implementor == null) {
+              // Reject methods with no implimentation
+              return false;

Review comment:
       We'll need to update the javadoc comment for this method to reflect these changes.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org