[CALCITE-7405] Pre-process expressions for correlations before building projection in SqlToRelConverter

[ian-bertolacci]

To circumvent the bloat optimizations, it is necessary to provide the correlation variables (if they exist) to the builder.
The main change here is to accomplish the logic done in SqlToRelConverter.getCorrelations without having built the Project node.
In this way we (a) extract the correlation id to use for the RelBuilder, and (b) normalize/resolve the correlation ids and rewrite the expressions; all before invoking the RelBuilder.

I've done my best to reuse existing logic. SqlToRelConverter.getCorrelations and SqlToRelConverter.massageExpressionsForCorrelation require a lot of the same logic for resolving the correlation variables for the current scope, so that was moved to a common utility method (SqlToRelConverter.getCorrelationInfo).

[xuzifu666]

Need to fix CI error first.

[xuzifu666]

Why change .gitignore file?

[ian-bertolacci]

When I run with intellij, git detected a ton of extra files.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
95.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[silundong]

If we create a temporary Project with LogicalProject.create before correlation detection, instead of using RelBuilder.projectNamed, would that resolve the issue? That could be much simpler.

[ian-bertolacci]

@silundong

If we create a temporary Project with LogicalProject.create before correlation detection, instead of using RelBuilder.projectNamed, would that resolve the issue? That could be much simpler.

Thats not an unreasonable suggestion, however I expect that change would also create a lot of test changes, both in this project and anything downstream.
I'll check and see (though it'll take me a few days).

[xiedeyantu]

I prefer a simpler, more intuitive processing logic, even if it affects some of the original plans. As you mentioned, are there other places where similar issues arise, and would it be better to have a unified logic for handling them?

[github-actions]

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.

[ian-bertolacci]

I'm hoping to get more reviews on this.

I prefer a simpler, more intuitive processing logic, even if it affects some of the original plans.

Thats fine, but I imagine that solution will take significant amount of time and effort (which I do not have for this project), and my preference (as I imagine is the preference of many others) is that we solve this severe bug we know about sooner, rather than wait for a better approach to correlation in (at a minimum) SqlToRelConverter.

[caicancai]

I'm hoping to get more reviews on this.

I prefer a simpler, more intuitive processing logic, even if it affects some of the original plans.

Thats fine, but I imagine that solution will take significant amount of time and effort (which I do not have for this project), and my preference (as I imagine is the preference of many others) is that we solve this severe bug we know about sooner, rather than wait for a better approach to correlation in (at a minimum) SqlToRelConverter.

@ian-bertolacci Hello, are you still working on this project? I can take some time to review it. Do you want to continue?

[ian-bertolacci]

Yes I would like it to be reviewed.

[caicancai]

.gitignore changes should be in a separate PR

[ian-bertolacci]

Moved to #4887

[caicancai]

The callback first assigns a null operation, which is then overridden within the if block. If someone later inserts code between the two assignments and uses the callback again, the incorrect (null operation) version will be used.

[ian-bertolacci]

I'll make this final and assign the no-op in an else block.

[caicancai]

private

[ian-bertolacci]

Changes made and commit rebased to current main head

[ian-bertolacci]

Where do we stand on this?

[mihaibudiu]

@silundong I think you are one of the most qualified to review this, if you have time

[silundong]

To circumvent the bloat optimizations, it is necessary to provide the correlation variables (if they exist) to the builder.

As I mentioned before, I create a temporary Project with LogicalProject.create before correlation detection, instead of using RelBuilder.projectNamed, just like:

RelNode project =
    LogicalProject.create(bb.root(), ImmutableList.of(), exprs, uniqueFieldNames,
        ImmutableSet.of());

final RelNode r;
final CorrelationUse p = getCorrelationUse(bb, project);
if (p != null) {
  assert p.r instanceof Project;
  // correlation variables have been normalized in p.r, we should use expressions
  // in p.r instead of the original exprs
  Project project1 = (Project) p.r;
  r = relBuilder.push(bb.root())
      .projectNamed(project1.getProjects(), uniqueFieldNames, true,
          ImmutableSet.of(p.id))
      .build();
} else {
  r = relBuilder.push(bb.root()).projectNamed(exprs, uniqueFieldNames, true).build();
}

This approach resolves the issue without causing any other test regressions. IMO, it's not only effective but also simpler and more intuitive. Do you see any drawbacks to it?

[ian-bertolacci]

@silundong that makes sense, and the added tests pass with your changes. I will push a commit with them.

Sorry that I did not understand this when you first made this comment several months ago.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud