Skip to content

[CALCITE-6193] If a query has more than one subexpression that matche…#3624

Open
JiajunBernoulli wants to merge 1 commit intoapache:mainfrom
JiajunBernoulli:CALCITE-6193
Open

[CALCITE-6193] If a query has more than one subexpression that matche…#3624
JiajunBernoulli wants to merge 1 commit intoapache:mainfrom
JiajunBernoulli:CALCITE-6193

Conversation

@JiajunBernoulli
Copy link
Contributor

…s a materialized view, only the first is substituted

…s a materialized view, only the first is substituted
@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

+ "LogicalCalc(expr#0..1=[{inputs}], deptno=[$t0])\n"
+ " LogicalJoin(condition=[=($0, $1)], joinType=[inner])\n"
+ " LogicalUnion(all=[true])\n"
+ " LogicalCalc(expr#0..1=[{inputs}], deptno=[$t1])\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

The 1879 and 1880 lines are still be replaced by mv's target descendant, it's parent may be shared with a wrong node, so I think this solution is not very is not very correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't commit, the RelNode is

LogicalCalc(expr#0..1=[{inputs}], deptno=[$t0])
  LogicalJoin(condition=[=($0, $1)], joinType=[inner])
    LogicalUnion(all=[true])
      LogicalCalc(expr#0..1=[{inputs}], deptno=[$t1]) -- same as 1879
        LogicalCalc(expr#0..4=[{inputs}], proj#0..1=[{exprs}]) -- same as 1880
          LogicalTableScan(table=[[hr, emps]])
      LogicalAggregate(group=[{1}])
        EnumerableTableScan(table=[[hr, MV0]])
    LogicalAggregate(group=[{0}])
      LogicalCalc(expr#0..4=[{inputs}], deptno=[$t1])
        LogicalTableScan(table=[[hr, emps]])

There are two LogicalCalc, do you mean this is Error?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, because these two LogicalCalc operators both are replaced by mv's target descendant, I think these temporary replacements should be undo after the real replacement has been done. This is the correct solution for this problem.

final MutableRel childOrNext =
queryDescendant.getInputs().isEmpty()
? next : queryDescendant.getInputs().get(0);
? next : queryDescendant.getInputs().get(currentInputIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think getInputs.get(currentInputIndex) is just a walk-around to force traverse the missed nodes. Actually, preOrderTraverseNext method can traverse all the nodes of this plan tree, There is no need to add a new status to record the current node's position.

Copy link
Contributor Author

@JiajunBernoulli JiajunBernoulli Jan 15, 2024

Choose a reason for hiding this comment

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

It is a demo.

I think that it always get(0) is error.
We should find a better way to traverse the missing nodes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think get(0) is correct, get(0) is just a starting point when traverse child-nodes, preOrderTraverseNext method can traverse the missing nodes.

@github-actions
Copy link

github-actions bot commented Dec 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants