You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "mihaibudiu (via GitHub)" <gi...@apache.org> on 2024/03/11 19:03:50 UTC

[PR] [CALCITE-6317] Optimization CoreRules.PROJECT_REDUCE_EXPRESSIONS is unsound [calcite]

mihaibudiu opened a new pull request, #3725:
URL: https://github.com/apache/calcite/pull/3725

   (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-6317] Optimization CoreRules.PROJECT_REDUCE_EXPRESSIONS is unsound [calcite]

Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on code in PR #3725:
URL: https://github.com/apache/calcite/pull/3725#discussion_r1520708118


##########
core/src/main/java/org/apache/calcite/rel/metadata/RelMdPredicates.java:
##########
@@ -382,7 +382,26 @@ public RelOptPredicateList getPredicates(Aggregate agg, RelMetadataQuery mq) {
 
     for (RexNode r : inputInfo.pulledUpPredicates) {
       ImmutableBitSet rCols = RelOptUtil.InputFinder.bits(r);
+      // We have to be careful for aggregates that have multiple groupSets.
+      // Consider this example:
+      // select deptno, sal, count(*)
+      // from emp where deptno = 10
+      // group by rollup(sal, deptno)
+      // Because of the ROLLUP, we cannot assume
+      // that deptno = 10 in the result: deptno may be NULL as well.
+      // Only if a field appears in all groupSets we can pull the predicate.
       if (groupKeys.contains(rCols)) {
+        boolean skip = false;

Review Comment:
   Using a single function to mark `skip` would look better than 395~400 lines.
   ```
   if (needSkip(rCols, agg)) {
     continue;
   }
   ```



-- 
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-6317] Optimization CoreRules.PROJECT_REDUCE_EXPRESSIONS is unsound [calcite]

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3725:
URL: https://github.com/apache/calcite/pull/3725#issuecomment-1989306753

   ## [![Quality Gate Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png 'Quality Gate Passed')](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3725) **Quality Gate passed**  
   Issues  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3725&resolved=false&inNewCodePeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/accepted-16px.png '') [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3725&metric=new_accepted_issues&view=list)
   
   Measures  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3725&resolved=false&inNewCodePeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3725&metric=new_coverage&view=list)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3725&metric=new_duplicated_lines_density&view=list)  
     
   [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3725)
   
   


-- 
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-6317] Optimization CoreRules.PROJECT_REDUCE_EXPRESSIONS is unsound [calcite]

Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3725:
URL: https://github.com/apache/calcite/pull/3725#issuecomment-1989915425

   @JiajunBernoulli thank you for the review; I made the suggested changes, and I have already squashed the commits in preparation for merging.


-- 
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-6317] Optimization CoreRules.PROJECT_REDUCE_EXPRESSIONS is unsound [calcite]

Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #3725:
URL: https://github.com/apache/calcite/pull/3725


-- 
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