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:
[email protected]


Reply via email to