[GitHub] [beam] brachi-wernick commented on a change in pull request #15728: [BEAM-12393] sql support for Zeta Sql

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

brachi-wernick commented on a change in pull request #15728:
URL: https://github.com/apache/beam/pull/15728#discussion_r730304342



##########
File path: sdks/java/extensions/zetasketch/src/main/java/org/apache/beam/sdk/extensions/zetasketch/HllCount.java
##########
@@ -279,6 +279,10 @@ private Builder(HllCountInitFn<InputT, ?> initFn) {
       public <K> Combine.PerKey<K, InputT, byte[]> perKey() {
         return Combine.perKey(initFn);
       }
+
+      public HllCountInitFn<InputT, ?> asUdaf() {

Review comment:
       Why is it better to return `Combine.CombineFn`? this `builder` is specific for `HllCount`?
   Also it is problematic to return `Combine.CombineFn<InputT, HyperLogLogPlusPlus<?>, byte[]>` because the `?` makes issues to compile:
   ```
    error: incompatible types: HllCountInitFn<InputT,CAP#1> cannot be converted to CombineFn<InputT,HyperLogLogPlusPlus<?>,byte[]>
           return initFn;
                  ^
     where InputT is a type-variable:
       InputT extends @org.checkerframework.checker.nullness.qual.Nullable Object declared in class Builder
     where CAP#1 is a fresh type-variable:
       CAP#1 extends Object from capture of ?
   1 error
   ```
   
   if I remove the `?` it also failed with
   ```
     missing type arguments for generic class HyperLogLogPlusPlus<V>
   ```

##########
File path: sdks/java/extensions/zetasketch/src/main/java/org/apache/beam/sdk/extensions/zetasketch/HllCount.java
##########
@@ -279,6 +279,10 @@ private Builder(HllCountInitFn<InputT, ?> initFn) {
       public <K> Combine.PerKey<K, InputT, byte[]> perKey() {
         return Combine.perKey(initFn);
       }
+
+      public HllCountInitFn<InputT, ?> asUdaf() {

Review comment:
       Why is it better to return `Combine.CombineFn`? this `builder` is specific for `HllCount`.
   
   Also it is problematic to return `Combine.CombineFn<InputT, HyperLogLogPlusPlus<?>, byte[]>` because the `?` makes issues to compile:
   ```
    error: incompatible types: HllCountInitFn<InputT,CAP#1> cannot be converted to CombineFn<InputT,HyperLogLogPlusPlus<?>,byte[]>
           return initFn;
                  ^
     where InputT is a type-variable:
       InputT extends @org.checkerframework.checker.nullness.qual.Nullable Object declared in class Builder
     where CAP#1 is a fresh type-variable:
       CAP#1 extends Object from capture of ?
   1 error
   ```
   
   if I remove the `?` it also failed with
   ```
     missing type arguments for generic class HyperLogLogPlusPlus<V>
   ```

##########
File path: sdks/java/extensions/zetasketch/src/main/java/org/apache/beam/sdk/extensions/zetasketch/ApproximateCountDistinct.java
##########
@@ -99,6 +99,10 @@
         .build();
   }
 
+  public static <T> HllCountInitFn<T, ?> getUdaf(TypeDescriptor<T> input) {

Review comment:
       yes works when putting `?` instead of `HyperLogLogPlusPlus`, less strict but fine enough :)
   commit this.




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

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

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