Conversation
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkSQLProperties.java
Show resolved
Hide resolved
| switch (op) { | ||
| case COUNT: | ||
| Count countAgg = (Count) aggregate; | ||
| assert (countAgg.column() instanceof NamedReference); |
There was a problem hiding this comment.
I kind of like not having if/else blocks compared to the remaining SparkAggregates utility we kept. Shall we adapt the one we kept to match this one? We probably just need to return null instead of throwing an exception.
Also, I see the kept utility has a special condition for isDistinct(). Is that still needed? We don't have it here.
There was a problem hiding this comment.
We still need the isDistinct(), because we don't want to push down count(distinct c)
| @@ -32,10 +33,14 @@ class SparkLocalScan implements LocalScan { | |||
| private final StructType readSchema; | |||
| private final InternalRow[] rows; | |||
|
|
|||
There was a problem hiding this comment.
nit: Shall we group all vars together? There is an empty line before filters now.
| @Override | ||
| public String toString() { | ||
| return String.format( | ||
| "IcebergScan(table=%s, type=%s, filters=%s)", |
There was a problem hiding this comment.
What about IcebergScan -> IcebergLocalScan to indicate it is a local scan?
| expressions.add((BoundAggregate<?, ?>) bound); | ||
| } else { | ||
| LOG.info( | ||
| "Skipping aggregate pushdown: AggregateFunc {} can't be converted to iceberg Expression", |
There was a problem hiding this comment.
nit: to iceberg Expression -> to Iceberg expression or simply to Iceberg.
|
Thanks, @huaxingao! |
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkAggregates.java
Outdated
Show resolved
Hide resolved
rdblue
left a comment
rdblue
left a comment
There was a problem hiding this comment.
Looks good to me other than the use of assert. Once that's rolled back I think this is ready to go.
|
Thanks, @huaxingao! |
|
Thank you all! |
3 participants
Address aggregate push down follow up comments