You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/10/17 00:48:56 UTC

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

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



##########
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:
       nit: You might have this return `Combine.CombineFn` and make it package private (drop the `public`)?

##########
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:
       nit: You might have this return `Combine.CombineFn`

##########
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:
       If this interface is package private, returning Combine.CombineFn doesn't really matter. If it is public, returning a more generic interface allows you to change the internals of the package without affecting the return type. To be more specific: If you change HllCountInitFn, anything calling into the package will have to recompile or will experience a NoSuchMethodError. If the return type is Combine.CombineFn, you can change HllCountInitFn without requiring calling packages to recompile.

##########
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:
       This specifically worked for me: `public static <T> Combine.CombineFn<T, ?, byte[]> getUdaf(TypeDescriptor<T> input) {`




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