[GitHub] [beam] ibzib commented on a change in pull request #13364: Implement logical_And as COmbineFn ZetaSQL

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

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



##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/BeamBuiltinAggregations.java
##########
@@ -397,6 +399,93 @@ public Long extractOutput(Long accum) {
    * <p>Note: null values are ignored when mixed with non-null values.
    * (https://issues.apache.org/jira/browse/BEAM-10379)
    */
+  //  static class BitAnd<T extends Number> extends CombineFn<T, Long, Long> {

Review comment:
       I think this commented-out implementation of BitAnd was left over from a stale commit. Please remove lines 402-488.

##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/BeamBuiltinAggregations.java
##########
@@ -426,11 +515,55 @@ public Long mergeAccumulators(Iterable<Long> accums) {
     }
 
     @Override
-    public Long extractOutput(Long accum) {
+    public Long extractOutput(Long accumulator) {
       if (this.isEmpty) {
         return null;
       }
-      return accum;
+      return accumulator;
+    }
+  }
+
+  static CombineFn createLogicalAnd(Schema.FieldType fieldType) {
+    if (fieldType.getTypeName() == TypeName.BOOLEAN) {
+      return new LogicalAnd();
+    }
+    throw new UnsupportedOperationException(
+        String.format("[%s] is not supported in LOGICAL_AND", fieldType));

Review comment:
       Nit: add to this message, "LOGICAL_AND only supports booleans."
   

##########
File path: sdks/java/extensions/sql/zetasql/src/test/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSqlDialectSpecTest.java
##########
@@ -3934,4 +3934,25 @@ public void testSimpleTableName() {
             Row.withSchema(singleField).addValues(15L).build());
     pipeline.run().waitUntilFinish(Duration.standardMinutes(PIPELINE_EXECUTION_WAITTIME_MINUTES));
   }
+
+  @Test
+  public void testLogicalAndZetaSQL() {
+    String sql =
+        "SELECT LOGICAL_AND(x) AS logical_and"
+            + " FROM UNNEST(["
+            + true

Review comment:
       Nit: It's a little strange to interpolate Java booleans `.toString()` in a SQL query, can you just make them strings instead?

##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/BeamBuiltinAggregations.java
##########
@@ -63,6 +63,8 @@
               .put("AVG", BeamBuiltinAggregations::createAvg)
               .put("BIT_OR", BeamBuiltinAggregations::createBitOr)
               // JIRA link:https://issues.apache.org/jira/browse/BEAM-10379
+              // .put("BIT_AND", BeamBuiltinAggregations::createBitAnd)

Review comment:
       Remove this line (BIT_AND is actually put into the table now in line 68, so the comment isn't needed).

##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/BeamBuiltinAggregations.java
##########
@@ -426,11 +515,55 @@ public Long mergeAccumulators(Iterable<Long> accums) {
     }
 
     @Override
-    public Long extractOutput(Long accum) {
+    public Long extractOutput(Long accumulator) {
       if (this.isEmpty) {
         return null;
       }
-      return accum;
+      return accumulator;
+    }
+  }
+
+  static CombineFn createLogicalAnd(Schema.FieldType fieldType) {
+    if (fieldType.getTypeName() == TypeName.BOOLEAN) {
+      return new LogicalAnd();
+    }
+    throw new UnsupportedOperationException(
+        String.format("[%s] is not supported in LOGICAL_AND", fieldType));
+  }
+
+  public static class LogicalAnd extends CombineFn<Boolean, Boolean, Boolean> {
+    private boolean isEmpty = true;
+
+    @Override
+    public Boolean createAccumulator() {
+      return Boolean.FALSE;
+    }
+
+    @Override
+    public Boolean addInput(Boolean mutableAccumulator, Boolean input) {
+      if (input != null) {
+        this.isEmpty = false;
+        return mutableAccumulator && input;
+      } else {
+        return null;

Review comment:
       The documentation for LOGICAL_AND says:
   
   "Returns the logical AND of all non-NULL expressions. Returns NULL if there are zero input rows or expression evaluates to NULL for all rows."
   
   In your current implementation, LOGICAL_AND would return NULL if *any* row was NULL. Please add some test with nulls to verify the correct behavior:
   
   1. returns null if there are no input rows
   2. returns null if all input rows are null
   3. returns logical AND of all non-NULL expressions if at least one input row is null and at least one input row is non-null
   
   https://github.com/google/zetasql/blob/master/docs/functions-and-operators.md#logical_and

##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/impl/BeamBuiltinMethods.java
##########
@@ -70,4 +70,7 @@
 
   public static final Method DATE_METHOD =
       Types.lookupMethod(DateFunctions.class, "date", Integer.class, Integer.class, Integer.class);
+
+  public static final Method LOGICAL_AND =

Review comment:
       This isn't used anywhere, is it? If not, please remove it.




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