xuzifu666 commented on PR #4371: URL: https://github.com/apache/calcite/pull/4371#issuecomment-2893921188
> > Thanks for all comments, had modified as follow: > > ``` > > 1. AggregateValuesRule is expanded to 2 modes, handling the default case and deduplication respectively; > > > > 2. Deduplication cannot be fully ensured for complex types such as Row, so deduplication is performed on non-complex data types such as Literal first to ensure that deduplication is effective; > > > > 3. Related tests are added > > please take a look for the changes. @mihaibudiu @asolimando @xiedeyantu > > ``` > > I won't push back on this, but I was more convinced on having the two modes activated conditionally in the `onMatch` method, like in the previous version. > > It's not always easy when to decide what makes two distinct rules or two optional behavior into the same one, as both approaches have pros and cons. > > Separate rules put more burden to the end-user and overhead in the planner, to me it's only worth it when there is a clear logical separation on the modes, that users might want to pick selectively (e.g., I want mode `A` but not mode `B`). > > Here I feel users would like any of the two optimizations, when their query matches the conditions, or none. I can't think of cases where one want one mode and not the other, so making two rules feels artificial. > > What do you guys think? @asolimando Thanks for your detailed review and I agree with you, I would changed it to previous version. -- 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]
