amogh-jahagirdar commented on code in PR #9176:
URL: https://github.com/apache/iceberg/pull/9176#discussion_r1408827906
##########
api/src/main/java/org/apache/iceberg/expressions/ValueAggregate.java:
##########
@@ -30,13 +32,16 @@ protected ValueAggregate(Operation op, BoundTerm<T> term) {
@Override
public T eval(StructLike struct) {
- return term().eval(struct);
+ Accessor<StructLike> leafAccessor =
Accessors.leafAccessor(term().ref().accessor());
+ return (T) leafAccessor.get(struct);
}
@Override
public T eval(DataFile file) {
- valueStruct.setValue(evaluateRef(file));
- return term().eval(valueStruct);
+ Accessor<StructLike> leafAccessor =
Accessors.leafAccessor(term().ref().accessor());
+ SingleValueStruct singleValueStruct = new SingleValueStruct();
+ singleValueStruct.setValue(evaluateRef(file));
Review Comment:
So the assumption here is that the leaf accessor is of the right type for
the aggregation. I think my main question is does this always hold true? I
think so because previously we would just go through a chain of accessors, and
now we're doing the same except skipping reading the StructLike.
Also do we need to always do a `term().eval` on a SingleValueStruct? I
removed that since ultimately for an aggregation pushdown I think this should
always just be a single primitive type which we should just be able to directly
access. Maybe @huaxingao has context here
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
