zabetak commented on PR #4013: URL: https://github.com/apache/calcite/pull/4013#issuecomment-2447414945
@julianhyde Putting minimality logic on every return of the `RelMdUniqueKeys` handler doesn't feel right to me. Even if the overhead is minimal why adding seemingly redundant code? Moreover, if the handler goes rogue and starts to generate not minimal keys at some place then chances are that we are going to fail before even arriving to the minimality check/filter. I don't mind adding the checks/fiters if you feel strongly about it but I see more cons than pros in this approach. For the record, we already have [RelMdUniqueKeys#filterSupersets](https://github.com/apache/calcite/blob/ed6c33eb1d27882b26fc16b995dde78de1709aff/core/src/main/java/org/apache/calcite/rel/metadata/RelMdUniqueKeys.java#L369) that is currently used by the `Aggregate` handler to ensure that keys are minimal. If decide to apply the filter in every other handler then I guess we don't need another method in `ImmutableBitSet` and probably don't need the minimality check in the tests either. -- 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]
