You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/10/27 03:43:20 UTC

[GitHub] [iceberg] huaxingao opened a new pull request, #6065: Fix TestAggregateBinding

huaxingao opened a new pull request, #6065:
URL: https://github.com/apache/iceberg/pull/6065

   Follow up [PR](https://github.com/apache/iceberg/pull/5961) and address a few comments:
   
   - https://github.com/apache/iceberg/pull/5961#discussion_r1004975423
   - https://github.com/apache/iceberg/pull/5961#discussion_r1005406682
   - https://github.com/apache/iceberg/pull/5961#discussion_r1005408297
   


-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] huaxingao commented on pull request #6065: Fix TestAggregateBinding

Posted by GitBox <gi...@apache.org>.
huaxingao commented on PR #6065:
URL: https://github.com/apache/iceberg/pull/6065#issuecomment-1295322595

   @rdblue Could you please take a look when you have a moment? Thanks a lot!


-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] rdblue commented on pull request #6065: Fix TestAggregateBinding

Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #6065:
URL: https://github.com/apache/iceberg/pull/6065#issuecomment-1304895651

   Thanks, @huaxingao!


-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] nastra commented on a diff in pull request #6065: Fix TestAggregateBinding

Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6065:
URL: https://github.com/apache/iceberg/pull/6065#discussion_r1006440948


##########
api/src/test/java/org/apache/iceberg/expressions/TestAggregateBinding.java:
##########
@@ -18,44 +18,28 @@
  */
 package org.apache.iceberg.expressions;
 
-import java.util.Arrays;
 import java.util.List;
 import org.apache.iceberg.exceptions.ValidationException;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
 import org.apache.iceberg.types.Types;
 import org.apache.iceberg.types.Types.StructType;
 import org.assertj.core.api.Assertions;
 import org.junit.Assert;
 import org.junit.Test;
 
 public class TestAggregateBinding {
-  private static final List<Expression.Operation> AGGREGATES =
-      Arrays.asList(Expression.Operation.COUNT, Expression.Operation.MAX, Expression.Operation.MIN);
+  private static final List<UnboundAggregate> list =

Review Comment:
   I noticed that in this entire file we're using `UnboundAggregate` without specifying a generic type. I think it would be good to replace all `UnboundAggregate` by `UnboundAggregate<String>`? Same for `BoundAggregate`, which should probably be `BoundAggregate<String, String>`



-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] huaxingao commented on a diff in pull request #6065: Fix TestAggregateBinding

Posted by GitBox <gi...@apache.org>.
huaxingao commented on code in PR #6065:
URL: https://github.com/apache/iceberg/pull/6065#discussion_r1007578132


##########
api/src/test/java/org/apache/iceberg/expressions/TestAggregateBinding.java:
##########
@@ -18,44 +18,28 @@
  */
 package org.apache.iceberg.expressions;
 
-import java.util.Arrays;
 import java.util.List;
 import org.apache.iceberg.exceptions.ValidationException;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
 import org.apache.iceberg.types.Types;
 import org.apache.iceberg.types.Types.StructType;
 import org.assertj.core.api.Assertions;
 import org.junit.Assert;
 import org.junit.Test;
 
 public class TestAggregateBinding {
-  private static final List<Expression.Operation> AGGREGATES =
-      Arrays.asList(Expression.Operation.COUNT, Expression.Operation.MAX, Expression.Operation.MIN);
+  private static final List<UnboundAggregate> list =

Review Comment:
   Fixed. Thanks! I accidentally put `String` type in the factory methods. It should be `T`. I fixed that.



-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] rdblue merged pull request #6065: Fix TestAggregateBinding

Posted by GitBox <gi...@apache.org>.
rdblue merged PR #6065:
URL: https://github.com/apache/iceberg/pull/6065


-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] huaxingao commented on pull request #6065: Fix TestAggregateBinding

Posted by GitBox <gi...@apache.org>.
huaxingao commented on PR #6065:
URL: https://github.com/apache/iceberg/pull/6065#issuecomment-1304926068

   Thanks! @rdblue @nastra @Fokko 


-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org