apache
[CALCITE-6586] Some Rules not firing due to RelMdPredicates returning null in VolcanoPlanner
The fundamental problem this PR solves is to make RelMetadataQuery#getPulledUpPredicates(RelSubset) not always return EMPTY, allowing some rules to be applied in VolcanoPlanner. However, after some rules were applied, there were some other issues that prevented the test from passing completely. So I split it into three commits for easy review
Some Druid tests have failed, and I will work on fixing them. It may take some time to run these tests locally, as I haven't done that before.
can you please fix the conflicts and squash the commits?
I will resolve these conflicts today. Also, do you think it would be easier to understand if we keep them as separate commit? It might make it clearer.
Having separate commits if they represent independent logical changes to the code. I think a good rule which tells you whether something could be broken into separate commits is: when you bisect the commits, the code should build and run fine between the commits. Otherwise they should not be separate.
Are you trying to close 4 issues with this PR? Ideally it should have been 4 separate PRs, especially if they are self-contained.
If no one objects, I will merge this as 4 commits that close 4 issues, but it is easy to miss the fact that the commits address separate issues - it's not the typical workflow. Keeping JIRA and github in sync is a non-trivial amount of work for maintainers, so it's good if we can make the task simpler.
If no one objects, I will merge this as 4 commits that close 4 issues, but it is easy to miss the fact that the commits address separate issues - it's not the typical workflow. Keeping JIRA and github in sync is a non-trivial amount of work for maintainers, so it's good if we can make the task simpler.
I apologize for the inconvenience caused. I will follow the correct workflow afterward. And I have compiled and tested each commit individually. Also, [CALCITE-6611] is only a temporary solution, so it shouldn't be closed yet.
So what should we do about the corresponding commit for 6611? Someone who reads the commit log may infer that the commits solves the problem. Maybe you can remove the issue number from the commit?
So what should we do about the corresponding commit for 6611? Someone who reads the commit log may infer that the commits solves the problem. Maybe you can remove the issue number from the commit?
I'm not sure what's best to do. But [CALCITE-1048] added a commit, and likewise added a case in "BUG", but it was still OPEN, so I did that as well. WDYT
To make sure: this PR is closing the following 3 issues: 6615, 6614, and 6586. Is that correct?
To make sure: this PR is closing the following 3 issues: 6615, 6614, and 6586. Is that correct?
Yes, that's correct! Also, I tried to resolve the conflict, but this caused some problems regarding pullUpPredicate being slow with too many values. I'll try to look into this.
you want to do this in the current PR? maybe I should just merge it
Thank you for your interest in this PR. But I don't think it could MERGE yet, as I don't have a good way of passing this test. I'll create a JIRA ISSUE to report this issue and then we'll evaluate what to do. WDYT
I have converted this to draft, please restore it when it's ready for review
Quality Gate passed
Issues
1 New issue
0 Accepted issues
Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code
See analysis details on SonarCloud
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
If no one else has comments I plan to merge this as 4 commits, which close 3 issues.