[ 
https://issues.apache.org/jira/browse/CALCITE-4668?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17370775#comment-17370775
 ] 

James Starr commented on CALCITE-4668:
--------------------------------------

[~julianhyde]

??James Starr, This seems like a useful change. But RelBuilder now scans the 
entire tree of the RHS of a join. That is potentially a Cartesian operation. 
Are you sure there are no performance issues in this change???

We already scan the entire left side of the join during creation, so generally 
this will be a (2 * N) increase in complexity, however, it depends on the size 
of left and right nodes.  Though checking the left side has been included in 
the 6 years now and has not been a problematic, it is unlikely that also 
checking the right side will problematic. 

??Can you improve the doc of isCorrelated describing what it returns? (When a 
method says “Checks...” I assume it’s going to throw.)??
These exceptions are validating arguments, and not expected to during 'normal' 
flow of the code.  I renamed it to checkIfCorrelated so it no longer collides 
with  uses bean property notation.

??Do you still require that there is precisely one correlation variable???
I did not change this.

??Add description to your test methods, explaining what they return and why. 
The reasoning is too subtle for a maintainer to understand.??
Done

??In your subject and commit message, change “exists” to “is used”.??
Done

??Several sub-query.iq tests are now trivial. That’s good news, I guess. But 
maybe it means that what used to be tested is no longer tested. So maybe you 
need to change those test queries or write new ones.??

The tests are checking the nullability of the equality check in a 'IN' clause.  
They are able to be simplified to a trivial plan because of the nullability, 
thus validating what they asserting.  If the test wanted to tests a particular 
chunk of code, then it should not of been written as a end to end test or at 
least should of stated such in the description of the test.

> RelBuilder.join should only emit a correlate when a correlate variable is used
> ------------------------------------------------------------------------------
>
>                 Key: CALCITE-4668
>                 URL: https://issues.apache.org/jira/browse/CALCITE-4668
>             Project: Calcite
>          Issue Type: Improvement
>          Components: core
>            Reporter: James Starr
>            Priority: Major
>              Labels: pull-request-available
>          Time Spent: 10m
>  Remaining Estimate: 0h
>
> We generate correlated join when no correlate variable is present on the 
> right side.  However, this could be simplified to emit a normal join.
> Given:
> {code:java}
> final RelNode rel = builder
>     .scan("EMP")
>     .variable(v)
>     .scan("DEPT")
>     .join(JoinRelType.LEFT,
>         builder.equals(builder.field(2, 0, "SAL"),
>             builder.literal(1000)),
>         ImmutableSet.of(v.get().id))
>     .build();
> {code}
> Currently the following is emitted:
> {code}
> LogicalFilter(condition=[=($7, $8)])
>   LogicalCorrelate(correlation=[$cor0], joinType=[left], 
> requiredColumns=[{0}])
>     LogicalTableScan(table=[[scott, EMP]])
>     LogicalFilter(condition=[=($cor0.DEPTNO, $0)])
>       LogicalTableScan(table=[[scott, DEPT]])
> {code}
> After the changes the following will be emitted:
> {code}
> LogicalJoin(condition=[=($7, $8)], joinType=[left])
>   LogicalTableScan(table=[[scott, EMP]])
>   LogicalTableScan(table=[[scott, DEPT]])
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Reply via email to