calcite icon indicating copy to clipboard operation
calcite copied to clipboard

[CALCITE-6340] RelBuilder drops set conventions when aggregating over duplicate projected fields

Open jduo opened this issue 1 year ago • 2 comments

Calculate the post-pruning RelTraitSet on the projection using TraitSet#apply(Mapping)

jduo avatar Apr 11 '24 22:04 jduo

@jduo, I have made some modifications to the PR in this commit, can you take a look?

I have added a conditional check on the results based on the status of the ticket you filed (as it's customary in Calcite), and I have also refined some tests and added few more

EDIT: finally it seems that all traits are dropped by this bug, so I suggest to rename the jira ticket as "[CALCITE-6340] RelBuilder drops traits when aggregating over duplicate projected fields" and update it in the final commit (when you will be squashing after getting approval) and wherever it is referenced in the code

I think the changes proposed are mostly OK, but the Bug entry should be for the new ticket (6391) instead of this one (6340).

jduo avatar May 09 '24 20:05 jduo

Few more minor changes and for me we are good to go.

Please don't force-push until the review process is on-going as it makes incremental reviewing more difficult.

In this case I force-pushed because there was a merge conflict against the head. Would it be preferred if I waited for the review to finish before dealing with these?

jduo avatar May 15 '24 17:05 jduo

Few more minor changes and for me we are good to go. Please don't force-push until the review process is on-going as it makes incremental reviewing more difficult.

In this case I force-pushed because there was a merge conflict against the head. Would it be preferred if I waited for the review to finish before dealing with these?

Unless for cases in which this is breaking CI then it's indeed preferable for the review process to be over.

asolimando avatar May 16 '24 17:05 asolimando

LGTM, thanks @jduo for taking care of the comments, can you squash the commits into a single one and make sure the commit message matches exactly the title of the Jira ticket preceded by [CALCITE-6340] (you can check the guidelines here if you are in doubt).

After that, I will wait for a couple more days in case someone has further comments, then I will merge.

Thanks @asolimando . I've squashed and updated the title.

jduo avatar May 16 '24 20:05 jduo