huaxingao commented on PR #16176: URL: https://github.com/apache/iceberg/pull/16176#issuecomment-6504014278
Thanks @hemanthboyina for working on this, and sorry for the slow review. I worked on this feature earlier in #6981, where Ryan gave some design feedback. My main comment on your PR is that the grouping logic itself doesn't depend on Spark, so I think it should live next to `AggregateEvaluator` instead of in `SparkScanBuilder`. That way other engines can reuse it, and we don't need a copy of `ArrayStructLike` in Spark. Here is a proposal: 1. I will put up a separate PR with the core part: an evaluator that takes the group-by fields and aggregates, checks the identity partition fields for each file's spec, and keeps one `AggregateEvaluator` per partition key. 2. You can then rebase this PR on top of it, so `SparkScanBuilder` only translates Spark's `Aggregation`, checks for row-level deletes, and builds the `SparkLocalScan`. What do you think? I'll tag you on the core PR once it's up. -- 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]
