add Aggregate Expressions #5961
Conversation
|
cc @rdblue @aokolnychyi @flyrain |
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/iceberg/expressions/UnboundAggregate.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/iceberg/expressions/UnboundAggregate.java
Outdated
Show resolved
Hide resolved
| case MIN: | ||
| return "min(" + term() + ")"; | ||
| default: | ||
| return "Aggregate is not supported: operation = " + op(); |
There was a problem hiding this comment.
Throw UnsupportedOperationException instead. It isn't correct to return a string like this in an expression.
api/src/test/java/org/apache/iceberg/expressions/TestAggregateBinding.java
Outdated
Show resolved
Hide resolved
api/src/test/java/org/apache/iceberg/expressions/TestAggregateBinding.java
Outdated
Show resolved
Hide resolved
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkAggregates.java
Show resolved
Hide resolved
|
|
||
| Max max = new Max(namedReference); | ||
| Expression expectedMax = Expressions.max(unquoted); | ||
| Expression acturalMax = SparkAggregates.convert(max); |
There was a problem hiding this comment.
Typo: "actural" -> "actual"
|
Thanks, @huaxingao! This is very close. Just a few minor things to fix. |
|
@rdblue Thank you very much for your review! I have addressed the comments. Could you please take one more look when you have time? Thanks! |
|
|
||
| @Test | ||
| public void testAggregateBinding() { | ||
| for (Expression.Operation op : AGGREGATES) { |
There was a problem hiding this comment.
Since AGGREGATES is only used here, I'd probably recommend refactoring so that you have a list of expressions for this loop:
private static final List<Expression> = ImmutableList.of(Expressions.count("x"), Expressions.max("x"), Expressions.min("x"));
...
for (Expression unbound : AGGREGATES) {
...
}
|
Thanks, @huaxingao! |
|
Thanks a lot! @rdblue I will refactor |
| try { | ||
| unbound.bind(struct, false); | ||
| Assert.fail("Binding a missing field should fail"); | ||
| } catch (ValidationException e) { |
There was a problem hiding this comment.
can we use Assertions.assertThatThrownBy here rather than a try-catch with Assert.fail?
| } | ||
|
|
||
| private static <T, C> BoundAggregate<T, C> assertAndUnwrapAggregate(Expression expr) { | ||
| Assert.assertTrue( |
There was a problem hiding this comment.
it might be worth to also update this to something like Assertions.assertThat(expr).isInstanceOf(BoundAggregate.class)
add Aggregate Expressions. These will be used for push down Max/Min/Count to Iceberg
Here is the original PR