You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "ViggoC (via GitHub)" <gi...@apache.org> on 2024/12/17 15:28:51 UTC
[PR] trim the input of aggregate in RelFieldTrimmer [calcite]
ViggoC opened a new pull request, #4095:
URL: https://github.com/apache/calcite/pull/4095
(no comment)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] Trim the input of aggregate in RelFieldTrimmer [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on PR #4095:
URL: https://github.com/apache/calcite/pull/4095#issuecomment-2550778508
@ViggoC I see, what about "[CALCITE-6734] RelFieldTrimmer should trim Aggregate's input fields which are arguments of unused aggregate functions" ?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] RelFieldTrimmer should trim Aggregate's input fields which are arguments of unused aggregate functions [calcite]
Posted by "ViggoC (via GitHub)" <gi...@apache.org>.
ViggoC commented on PR #4095:
URL: https://github.com/apache/calcite/pull/4095#issuecomment-2551109628
@ILuffZhe Thank you for the reminder.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] Trim the input of aggregate in RelFieldTrimmer [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #4095:
URL: https://github.com/apache/calcite/pull/4095#discussion_r1889787936
##########
core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java:
##########
@@ -1055,15 +1055,19 @@ public TrimResult trimFields(
final ImmutableBitSet.Builder inputFieldsUsed =
aggregate.getGroupSet().rebuild();
// 2. agg functions
+ int index = aggregate.getGroupCount();
Review Comment:
nit: I think it would be more clear if this variable was named something like `int aggCallIndex`
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] RelFieldTrimmer should trim Aggregate's input fields which are arguments of unused aggregate functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4095:
URL: https://github.com/apache/calcite/pull/4095#issuecomment-2551133006
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4095) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4095&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4095&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4095&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4095&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4095&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4095)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] RelFieldTrimmer should trim Aggregate's input fields which are arguments of unused aggregate functions [calcite]
Posted by "ILuffZhe (via GitHub)" <gi...@apache.org>.
ILuffZhe merged PR #4095:
URL: https://github.com/apache/calcite/pull/4095
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] RelFieldTrimmer should trim Aggregate's input fields which are arguments of unused aggregate functions [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on PR #4095:
URL: https://github.com/apache/calcite/pull/4095#issuecomment-2551036574
@ViggoC LGTM.
There is a checkStyle error (line too long) to be fixed.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] Trim the input of aggregate in RelFieldTrimmer [calcite]
Posted by "ViggoC (via GitHub)" <gi...@apache.org>.
ViggoC commented on code in PR #4095:
URL: https://github.com/apache/calcite/pull/4095#discussion_r1889632489
##########
core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java:
##########
@@ -1055,15 +1055,19 @@ public TrimResult trimFields(
final ImmutableBitSet.Builder inputFieldsUsed =
aggregate.getGroupSet().rebuild();
// 2. agg functions
+ int index = aggregate.getGroupCount();
for (AggregateCall aggCall : aggregate.getAggCallList()) {
- inputFieldsUsed.addAll(aggCall.getArgList());
- if (aggCall.filterArg >= 0) {
- inputFieldsUsed.set(aggCall.filterArg);
- }
- if (aggCall.distinctKeys != null) {
- inputFieldsUsed.addAll(aggCall.distinctKeys);
+ if (fieldsUsed.get(index)) {
Review Comment:
Because all group by keys are necessary in aggregate no matter if they are used in the consumer. If the top Project contains ENAME, `fieldsUsed.get(indexOfEname)` will be true.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] Trim the input of aggregate in RelFieldTrimmer [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4095:
URL: https://github.com/apache/calcite/pull/4095#issuecomment-2548961015
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4095) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4095&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4095&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4095&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4095&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4095&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4095)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] Trim the input of aggregate in RelFieldTrimmer [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4095:
URL: https://github.com/apache/calcite/pull/4095#issuecomment-2548994032
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4095) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4095&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4095&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4095&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4095&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4095&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4095)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] Trim the input of aggregate in RelFieldTrimmer [calcite]
Posted by "ILuffZhe (via GitHub)" <gi...@apache.org>.
ILuffZhe commented on code in PR #4095:
URL: https://github.com/apache/calcite/pull/4095#discussion_r1889444233
##########
core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java:
##########
@@ -1055,15 +1055,19 @@ public TrimResult trimFields(
final ImmutableBitSet.Builder inputFieldsUsed =
aggregate.getGroupSet().rebuild();
// 2. agg functions
+ int index = aggregate.getGroupCount();
for (AggregateCall aggCall : aggregate.getAggCallList()) {
- inputFieldsUsed.addAll(aggCall.getArgList());
- if (aggCall.filterArg >= 0) {
- inputFieldsUsed.set(aggCall.filterArg);
- }
- if (aggCall.distinctKeys != null) {
- inputFieldsUsed.addAll(aggCall.distinctKeys);
+ if (fieldsUsed.get(index)) {
Review Comment:
I'm not familiar here, just one small question: Why do the index start with aggregate's groupCount? In this case, if the top Project contains ENAME, can it be covered?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] Trim the input of aggregate in RelFieldTrimmer [calcite]
Posted by "ViggoC (via GitHub)" <gi...@apache.org>.
ViggoC commented on PR #4095:
URL: https://github.com/apache/calcite/pull/4095#issuecomment-2550802216
@rubenada Make sense, I'll revise it. Thx
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] RelFieldTrimmer should trim Aggregate's input fields which are arguments of unused aggregate functions [calcite]
Posted by "ILuffZhe (via GitHub)" <gi...@apache.org>.
ILuffZhe commented on PR #4095:
URL: https://github.com/apache/calcite/pull/4095#issuecomment-2551028436
@ViggoC you don't need to squash your commits every time before the final merging. And it helps other reviewers to see the differences, 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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] Trim the input of aggregate in RelFieldTrimmer [calcite]
Posted by "ILuffZhe (via GitHub)" <gi...@apache.org>.
ILuffZhe commented on code in PR #4095:
URL: https://github.com/apache/calcite/pull/4095#discussion_r1889638288
##########
core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java:
##########
@@ -1055,15 +1055,19 @@ public TrimResult trimFields(
final ImmutableBitSet.Builder inputFieldsUsed =
aggregate.getGroupSet().rebuild();
// 2. agg functions
+ int index = aggregate.getGroupCount();
for (AggregateCall aggCall : aggregate.getAggCallList()) {
- inputFieldsUsed.addAll(aggCall.getArgList());
- if (aggCall.filterArg >= 0) {
- inputFieldsUsed.set(aggCall.filterArg);
- }
- if (aggCall.distinctKeys != null) {
- inputFieldsUsed.addAll(aggCall.distinctKeys);
+ if (fieldsUsed.get(index)) {
Review Comment:
Thanks for the clarify
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] RelFieldTrimmer should trim Aggregate's input fields which are arguments of unused aggregate functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4095:
URL: https://github.com/apache/calcite/pull/4095#issuecomment-2551417841
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4095) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4095&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4095&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4095&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4095&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4095&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4095)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] Trim the input of aggregate in RelFieldTrimmer [calcite]
Posted by "ViggoC (via GitHub)" <gi...@apache.org>.
ViggoC commented on PR #4095:
URL: https://github.com/apache/calcite/pull/4095#issuecomment-2550670007
@rubenada Actually, I think **aggregate functions that are not used by the consumer** is already trimmed. What this MR do is trimming the unused input fields of aggregate function.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] Trim the input of aggregate in RelFieldTrimmer [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #4095:
URL: https://github.com/apache/calcite/pull/4095#discussion_r1889786853
##########
core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java:
##########
@@ -1055,15 +1055,19 @@ public TrimResult trimFields(
final ImmutableBitSet.Builder inputFieldsUsed =
aggregate.getGroupSet().rebuild();
// 2. agg functions
+ int index = aggregate.getGroupCount();
for (AggregateCall aggCall : aggregate.getAggCallList()) {
- inputFieldsUsed.addAll(aggCall.getArgList());
- if (aggCall.filterArg >= 0) {
- inputFieldsUsed.set(aggCall.filterArg);
- }
- if (aggCall.distinctKeys != null) {
- inputFieldsUsed.addAll(aggCall.distinctKeys);
+ if (fieldsUsed.get(index)) {
Review Comment:
I agree with @ILuffZhe that the new code, while correct, is not straightforward to understand, so perhaps a comment will be useful here. I'd suggest maybe updating the comment line 1057 with something like:
`// 2. agg functions: consider only the ones that are needed according to fieldsUsed`
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] RelFieldTrimmer should trim Aggregate's input fields which are arguments of unused aggregate functions [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on PR #4095:
URL: https://github.com/apache/calcite/pull/4095#issuecomment-2551352976
LGTM. @ViggoC could you please squash commits into a single one to prepare the merge (I plan to proceed on the next 24-48h if no further remark arrives)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6734] Trim the input of aggregate in RelFieldTrimmer [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on PR #4095:
URL: https://github.com/apache/calcite/pull/4095#issuecomment-2550626972
@ViggoC Patch looks good. As suggested in the Jira ticket, I think the commit message (and Jira title) could be improved, e.g. something along the lines `[CALCITE-6734] RelFieldTrimmer should trim aggregate functions that are not used by the consumer`. This should be reflected on the test's Javadoc as well.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org