Skip to content

Conversation

@JingDas
Copy link

@JingDas JingDas commented Jun 14, 2023

This feature is mainly about expanding ProjectJoinRemoveRule to support inner join remove
by the foreign-unique constraint in the catalog.

The main steps are as follows:

  1. Analyse the column used by project, and then split them to left and right side.
  2. Acccording to the project info above and outer join type, bail out in some scene.
  3. Get join info such as join keys.
  4. For inner join check foreign and unique keys, these may use
    RelMetadataQuery#getForeignKeys(newly add, similar to RelMetadataQuery#getUniqueKeys),
    RelOptTable#getReferentialConstraints.
  5. Check removing side join keys are areColumnsUnique both for outer join and inner join.
  6. If all done, calculate the fianl project and transform.

Test cases are added to RelOptRulesTest and RelMetadataTest.

@JingDas JingDas requested a review from asolimando July 10, 2023 12:14
@asolimando
Copy link
Member

The RelOptForeignKey component has been added to represent the composite or single foreign key and the unique constraint that foreign key references. Sorry to bother you, @asolimando @julianhyde would you help me to review the code?

Thanks @JingDas, I will review again as soon as I have some time.

In the meantime, I have approved the pending workflows, but I have also noticed that there are 18 code smells detected by SonarCloud, could you do a pass on this list and fix whatever can be reasonably fixed?

As for any automatic tool not everything makes sense and there might even be false positives.

@JingDas
Copy link
Author

JingDas commented Jul 12, 2023

The RelOptForeignKey component has been added to represent the composite or single foreign key and the unique constraint that foreign key references. Sorry to bother you, @asolimando @julianhyde would you help me to review the code?

Thanks @JingDas, I will review again as soon as I have some time.

In the meantime, I have approved the pending workflows, but I have also noticed that there are 18 code smells detected by SonarCloud, could you do a pass on this list and fix whatever can be reasonably fixed?

As for any automatic tool not everything makes sense and there might even be false positives.

@JingDas JingDas closed this Jul 12, 2023
@JingDas JingDas reopened this Jul 12, 2023
@JingDas
Copy link
Author

JingDas commented Jul 12, 2023

Sorry, it was closed by mistake,I reopen it and fixed the most necessary SonarCloud detected code.

@JingDas
Copy link
Author

JingDas commented Jul 17, 2023

I some confusion about the failed results of the CI tests.
CI failed test log as fllowing:

org.apache.calcite.test.RelOptRulesTest > testProjectJoinRemove25() failure marker
FAILURE   0.0sec, org.apache.calcite.test.RelOptRulesTest > testProjectJoinRemove25()
    java.lang.AssertionError: Expected planAfter must not be present when using unchanged=true or calling checkUnchanged.
        at org.apache.calcite.test.RelOptFixture.checkPlanning(RelOptFixture.java:397)
        at org.apache.calcite.test.RelOptFixture.check(RelOptFixture.java:330)
        at org.apache.calcite.test.RelOptFixture.checkUnchanged(RelOptFixture.java:322)
        at org.apache.calcite.test.RelOptRulesTest.testProjectJoinRemove25(RelOptRulesTest.java:6297)

In RelOptFixture.java:397 it throw the exception.
But in RelOptRulesTest#testProjectJoinRemove25, I use checkUnchanged() method which means unchanged is true. It should run into RelOptFixture.java:395 instead of RelOptFixture.java:397.

@JingDas
Copy link
Author

JingDas commented Jul 17, 2023

Aha! It seems calcite main code change, And the CI seems to use the new main logic. I will fix it according to main code usage.

@JingDas
Copy link
Author

JingDas commented Jul 17, 2023

Aha! It seems calcite main code change, And the CI seems to use the new main logic. I will fix it according to main code usage.

Have fixed it.

…ove by the foreign-unique constraint in the catalog
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

83.8% 83.8% Coverage
0.0% 0.0% Duplication

@mihaibudiu
Copy link
Contributor

This PR was approved, but never merged. What's the plan?

@asolimando asolimando removed the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jul 12, 2024
Copy link
Member

@asolimando asolimando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mihaibudiu, discussion in the associated Jira ticket led to creating CALCITE-5881, which needs to be addressed before getting this in, unfortunately there was no progress on it for a long time now. I will remove the approval to make the situation clearer, thanks for the reminder.

@github-actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.

@github-actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.

@github-actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jun 29, 2025
@github-actions github-actions bot removed the stale label Sep 8, 2025
@github-actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

request review request a review from committers/contributors stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants