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? does this always hold true is the question. 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
