[GitHub] [beam] apilloud commented on a change in pull request #14263: [BEAM-10943] Enable SqlTransform::registerUdf in ZetaSQL.

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

apilloud commented on a change in pull request #14263:
URL: https://github.com/apache/beam/pull/14263#discussion_r604324748



##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/BeamZetaSqlCatalog.java
##########
@@ -276,6 +285,49 @@ private void addWindowTvfs() {
             null));
   }
 
+  private void addUdfsFromSchema() {
+    for (String functionName : calciteSchema.getFunctionNames()) {
+      Collection<org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.schema.Function>
+          functions = calciteSchema.getFunctions(functionName);
+      if (functions.size() != 1) {
+        throw new IllegalArgumentException(
+            String.format(
+                "Expected exactly 1 definition for function '%s', but found %d.",

Review comment:
       Is this an assert or a Beam ZetaSQL limitation? Is the case where we get multiple values back is when the user supplied multiple variants of the same function? If so: `Beam ZetaSQL supports exactly 1...`
   
   Users are going to eventually ask for multiple variants of the same UDF for different types, it appears that is already supported by Calcite and ZetaSQL.

##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/BeamZetaSqlCatalog.java
##########
@@ -276,6 +285,49 @@ private void addWindowTvfs() {
             null));
   }
 
+  private void addUdfsFromSchema() {
+    for (String functionName : calciteSchema.getFunctionNames()) {
+      Collection<org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.schema.Function>
+          functions = calciteSchema.getFunctions(functionName);
+      if (functions.size() != 1) {
+        throw new IllegalArgumentException(
+            String.format(
+                "Expected exactly 1 definition for function '%s', but found %d.",
+                functionName, functions.size()));
+      }
+      for (org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.schema.Function function :
+          functions) {
+        if (function instanceof ScalarFunctionImpl) {

Review comment:
       Are there other cases we should be concerned with? If there is nothing, should we add an `else` case that throws an exception? (I'm worried about cases where UDFs are silently dropped.)




-- 
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