Re: [PR] [CALCITE-6261] AssertionError with field pruning & duplicate agg calls [calcite]
mihaibudiu merged PR #3703: URL: https://github.com/apache/calcite/pull/3703 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6261] AssertionError with field pruning & duplicate agg calls [calcite]
rubenada commented on PR #3703: URL: https://github.com/apache/calcite/pull/3703#issuecomment-1964164955 LGTM -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6261] AssertionError with field pruning & duplicate agg calls [calcite]
sonarcloud[bot] commented on PR #3703: URL: https://github.com/apache/calcite/pull/3703#issuecomment-1963773636 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3703) **Quality Gate passed** Issues  [4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3703=false=true) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3703=false=true)  [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3703=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3703=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3703) -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6261] AssertionError with field pruning & duplicate agg calls [calcite]
nielspardon commented on code in PR #3703: URL: https://github.com/apache/calcite/pull/3703#discussion_r1502347632 ## core/src/main/java/org/apache/calcite/tools/RelBuilder.java: ## @@ -2510,7 +2510,7 @@ private RelBuilder aggregate_(GroupKeyImpl groupKey, // duplicates, then add a Project. final Set callSet = new HashSet<>(); final PairList projects = PairList.of(); -Util.range(groupSet.cardinality()) +Util.range(groupSet2.cardinality()) Review Comment: - I renamed `groupSet2` and `groupSets2` to `groupSetAfterPruning` and `groupSetsAfterPruning` respectively to give them more obvious names. - I also renamed the helper variable `map` to `sourceFieldToTargetFieldMap` to make it more obvious. - I moved the field pruning and agg call deduplication code into its own `pruneAggregateInputFieldsAndDeduplicateAggCalls ` method to reduce the length of the original `aggregate_()` method. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6261] AssertionError with field pruning & duplicate agg calls [calcite]
nielspardon commented on code in PR #3703: URL: https://github.com/apache/calcite/pull/3703#discussion_r1501124361 ## core/src/main/java/org/apache/calcite/tools/RelBuilder.java: ## @@ -2510,7 +2510,7 @@ private RelBuilder aggregate_(GroupKeyImpl groupKey, // duplicates, then add a Project. final Set callSet = new HashSet<>(); final PairList projects = PairList.of(); -Util.range(groupSet.cardinality()) +Util.range(groupSet2.cardinality()) Review Comment: I can give it a shot on Monday when I'm back in the office. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6261] AssertionError with field pruning & duplicate agg calls [calcite]
mihaibudiu commented on code in PR #3703: URL: https://github.com/apache/calcite/pull/3703#discussion_r1501123137 ## core/src/main/java/org/apache/calcite/tools/RelBuilder.java: ## @@ -2510,7 +2510,7 @@ private RelBuilder aggregate_(GroupKeyImpl groupKey, // duplicates, then add a Project. final Set callSet = new HashSet<>(); final PairList projects = PairList.of(); -Util.range(groupSet.cardinality()) +Util.range(groupSet2.cardinality()) Review Comment: I have approved the PR, but if you are willing to improve the code I will wait for a new commit. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6261] AssertionError with field pruning & duplicate agg calls [calcite]
nielspardon commented on code in PR #3703: URL: https://github.com/apache/calcite/pull/3703#discussion_r1501122201 ## core/src/main/java/org/apache/calcite/tools/RelBuilder.java: ## @@ -2510,7 +2510,7 @@ private RelBuilder aggregate_(GroupKeyImpl groupKey, // duplicates, then add a Project. final Set callSet = new HashSet<>(); final PairList projects = PairList.of(); -Util.range(groupSet.cardinality()) +Util.range(groupSet2.cardinality()) Review Comment: > Sure, but this means that #3700 is actually equivalent to this fix. > So that PR probably fixes both issues as well. Yes, functionally the other PR does fix the AssertionError and the functional outcome is the same. > Is there a more descriptive name that could be used instead of groupSet2? Maybe that's the root problem: it is very hard to tell what groupSet2 means. But if it was called e.g., replacementGroupSet, then it would become apparent where it is misused. Yes, I also think the problem was introduced because of the ambiguity of the variables in the first place. A first thought I had was that maybe some refactoring of this method into smaller methods like extracting the agg call deduplication code into its own method could help to have a clearer separation with less ambiguity. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6261] AssertionError with field pruning & duplicate agg calls [calcite]
mihaibudiu commented on code in PR #3703: URL: https://github.com/apache/calcite/pull/3703#discussion_r1501115489 ## core/src/main/java/org/apache/calcite/tools/RelBuilder.java: ## @@ -2510,7 +2510,7 @@ private RelBuilder aggregate_(GroupKeyImpl groupKey, // duplicates, then add a Project. final Set callSet = new HashSet<>(); final PairList projects = PairList.of(); -Util.range(groupSet.cardinality()) +Util.range(groupSet2.cardinality()) Review Comment: Sure, but this means that #3700 is actually equivalent to this fix. So that PR probably fixes both issues as well. Is there a more descriptive name that could be used instead of groupSet2? Maybe that's the root problem: it is very hard to tell what groupSet2 means. But if it was called e.g., replacementGroupSet, then it would become apparent where it is misused. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6261] AssertionError with field pruning & duplicate agg calls [calcite]
nielspardon commented on code in PR #3703: URL: https://github.com/apache/calcite/pull/3703#discussion_r1501082800 ## core/src/main/java/org/apache/calcite/tools/RelBuilder.java: ## @@ -2510,7 +2510,7 @@ private RelBuilder aggregate_(GroupKeyImpl groupKey, // duplicates, then add a Project. final Set callSet = new HashSet<>(); final PairList projects = PairList.of(); -Util.range(groupSet.cardinality()) +Util.range(groupSet2.cardinality()) Review Comment: From my understanding the return value of `groupSet.cardinality()` should always be the same as for `groupSet2.cardinality()` in the current implementation. My main point is that with all the logic at the beginning of the `aggregate_()` method which introduces `groupSet2` we should use it consistently in the latter part of the method. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6261] AssertionError with field pruning & duplicate agg calls [calcite]
mihaibudiu commented on code in PR #3703: URL: https://github.com/apache/calcite/pull/3703#discussion_r1501071624 ## core/src/main/java/org/apache/calcite/tools/RelBuilder.java: ## @@ -2510,7 +2510,7 @@ private RelBuilder aggregate_(GroupKeyImpl groupKey, // duplicates, then add a Project. final Set callSet = new HashSet<>(); final PairList projects = PairList.of(); -Util.range(groupSet.cardinality()) +Util.range(groupSet2.cardinality()) Review Comment: when is groupSet.cardinality() != groupSet2.cardinality()? -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6261] AssertionError with field pruning & duplicate agg calls [calcite]
mihaibudiu commented on code in PR #3703: URL: https://github.com/apache/calcite/pull/3703#discussion_r1501071624 ## core/src/main/java/org/apache/calcite/tools/RelBuilder.java: ## @@ -2510,7 +2510,7 @@ private RelBuilder aggregate_(GroupKeyImpl groupKey, // duplicates, then add a Project. final Set callSet = new HashSet<>(); final PairList projects = PairList.of(); -Util.range(groupSet.cardinality()) +Util.range(groupSet2.cardinality()) Review Comment: when is groupSet.cardinality() != groupset2.cardinality()? -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-6261] AssertionError with field pruning & duplicate agg calls [calcite]
sonarcloud[bot] commented on PR #3703: URL: https://github.com/apache/calcite/pull/3703#issuecomment-1961127031 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3703) **Quality Gate passed** Issues  [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3703=false=true) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3703=false=true)  [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3703=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3703=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3703) -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
