[CALCITE-7479] Remove redundant aggregate group keys with FD#4892
[CALCITE-7479] Remove redundant aggregate group keys with FD#4892xiedeyantu wants to merge 1 commit intoapache:mainfrom
Conversation
|
Just draft. |
xiedeyantu
force-pushed
the
agg-remove
branch
3 times, most recently
from
137af74 to
ebcde09
Compare
xiedeyantu
changed the title
mihaibudiu
left a comment
mihaibudiu
left a comment
There was a problem hiding this comment.
This looks fine.
How come it is never applicable?
Are all our tests using optimal group by keys?
Or we just don't have many tables with keys?
"How come it is never applicable?" I didn't quite understand the meaning of this sentence. Because the functional dependencies we currently support are limited, I used the primary key as an example. |
|
No other tests were modified |
|
I'm concerned about whether this rewrite is meaningful, as it requires the introduction of |
|
I think there are other uses of |
Because this new rule is not set as a "default" rule, it doesn't have any other impact. |
|
Can you make it default and see what happens? |
Are you suggesting that I don't need to worry too much about "single_value"? Can the compiler do more? |
Of course, I can create a new branch to add it as a default rule and observe the outcome later. |
|
Doesn't even have to be a branch, you can run the experiment and report the results. |
|
There are lots of uses of SINGLE_VALUE, I think that one is fine. |
|
@mihaibudiu If you have time, could you please take another look at this PR? |
|
I have approved it already. Anything in particular you'd like me to check? |
|
Since there were some changes in the latest commit, I wanted to see if you need to confirm it again, even though it followed the solution discussed in the other PR. |
| * <p>{@code ANY_VALUE} is chosen over {@code SINGLE_VALUE} because functional | ||
| * dependence only guarantees that a removed key maps to a single <em>value</em> | ||
| * per group, not that the group contains a single <em>row</em>. | ||
| * {@code SINGLE_VALUE} enforces the stricter, row-count constraint at runtime |
There was a problem hiding this comment.
I think this explanation is not necessary. If I was confused, it doesn't mean other people will as well. In fact, SINGLE_VALUE is documented properly.
There was a problem hiding this comment.
What do you think about me attaching the examples and explanations from that PR to the Jira ticket, and then removing this section of explanation from the rules?
|
xiedeyantu
added
the
LGTM-will-merge-soon
2 participants
Jira Link
CALCITE-7479
Changes Proposed