[jira] [Commented] (CALCITE-2997) Avoid pushing down join condition in SqlToRelConverter
[
https://issues.apache.org/jira/browse/CALCITE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17113462#comment-17113462
]
Julian Hyde commented on CALCITE-2997:
--
I support adding config options to SqlToRelConverter so that people can choose
to do more or less optimization. But I'd keep the default behavior the same as
it is today.
Generally it's good to add 'no-brainer' optimizations to RelBuilder, especially
if they tend to produce a RelNode tree that is more concise. (CALCITE-3763 is a
great example of this - note that it modifies an existing Project, but never
adds one.) But it's not an absolute rule.
And it's almost always good to move 'optimizations' done by SqlToRelConverter
into RelBuilder (provided this can be done without changing behavior).
> Avoid pushing down join condition in SqlToRelConverter
> --
>
> Key: CALCITE-2997
> URL: https://issues.apache.org/jira/browse/CALCITE-2997
> Project: Calcite
> Issue Type: Bug
>Reporter: Jin Xing
>Assignee: Julian Hyde
>Priority: Major
>
> In current code, *SqlToRelConverter:createJoin* is calling
> *RelOptUtil.pushDownJoinConditions* for optimization. And we can find below
> conversion from *SqlNode* to *RelNode*:
> {code:java}
> SqlNode:
> select * from A join B on A.x = B.x * 2
> RelNode (Logical-Plan):
> Join (condition:col0=col1)
> |-Project(x as col0)
> | |-Scan(A)
> |-Project(x * 2 as col1)
> |-Scan(B){code}
> As we can see the logical plan(*RelNode*) posted above is not the pure
> reflection of the original SQL String(*SqlNode*). The optimization is mixed
> into the phase on which AST is converted to Logical-Plan. Actually optimizing
> rule of JoinPushExpressionsRule is doing exactly the same kind of thing.
> Shall we just keep the optimization inside Optimized-Logical-Plan ? I mean
> shall we avoid calling *RelOptUtil.pushDownJoinConditions* in
> *SqlToRelConverter:createJoin*
> I raised this issue because that we are doing something based on the
> Logical-Plan. And it makes us really confused that the Logical-Plan doesn't
> corresponds to SqlNode.
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-2997) Avoid pushing down join condition in SqlToRelConverter
[
https://issues.apache.org/jira/browse/CALCITE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17112733#comment-17112733
]
Chunwei Lei commented on CALCITE-2997:
--
I think It may be less overhead if such optimizations are done in
SqltoRelConverter though it is a little weird. But I'm fine with the PR. Should
we also move other optimization mentioned by [~jinxing6...@126.com] to
RelBuilder too?
> Avoid pushing down join condition in SqlToRelConverter
> --
>
> Key: CALCITE-2997
> URL: https://issues.apache.org/jira/browse/CALCITE-2997
> Project: Calcite
> Issue Type: Bug
>Reporter: Jin Xing
>Assignee: Julian Hyde
>Priority: Major
>
> In current code, *SqlToRelConverter:createJoin* is calling
> *RelOptUtil.pushDownJoinConditions* for optimization. And we can find below
> conversion from *SqlNode* to *RelNode*:
> {code:java}
> SqlNode:
> select * from A join B on A.x = B.x * 2
> RelNode (Logical-Plan):
> Join (condition:col0=col1)
> |-Project(x as col0)
> | |-Scan(A)
> |-Project(x * 2 as col1)
> |-Scan(B){code}
> As we can see the logical plan(*RelNode*) posted above is not the pure
> reflection of the original SQL String(*SqlNode*). The optimization is mixed
> into the phase on which AST is converted to Logical-Plan. Actually optimizing
> rule of JoinPushExpressionsRule is doing exactly the same kind of thing.
> Shall we just keep the optimization inside Optimized-Logical-Plan ? I mean
> shall we avoid calling *RelOptUtil.pushDownJoinConditions* in
> *SqlToRelConverter:createJoin*
> I raised this issue because that we are doing something based on the
> Logical-Plan. And it makes us really confused that the Logical-Plan doesn't
> corresponds to SqlNode.
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-2997) Avoid pushing down join condition in SqlToRelConverter
[
https://issues.apache.org/jira/browse/CALCITE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17111710#comment-17111710
]
Haisheng Yuan commented on CALCITE-2997:
I agree with [~jinxing6...@126.com]. Instead of providing an option to disable
push down join condition, shall we consider the option to disable the
optimization, return the original logical representation of sqlnode?
> Avoid pushing down join condition in SqlToRelConverter
> --
>
> Key: CALCITE-2997
> URL: https://issues.apache.org/jira/browse/CALCITE-2997
> Project: Calcite
> Issue Type: Bug
>Reporter: Jin Xing
>Assignee: Julian Hyde
>Priority: Major
>
> In current code, *SqlToRelConverter:createJoin* is calling
> *RelOptUtil.pushDownJoinConditions* for optimization. And we can find below
> conversion from *SqlNode* to *RelNode*:
> {code:java}
> SqlNode:
> select * from A join B on A.x = B.x * 2
> RelNode (Logical-Plan):
> Join (condition:col0=col1)
> |-Project(x as col0)
> | |-Scan(A)
> |-Project(x * 2 as col1)
> |-Scan(B){code}
> As we can see the logical plan(*RelNode*) posted above is not the pure
> reflection of the original SQL String(*SqlNode*). The optimization is mixed
> into the phase on which AST is converted to Logical-Plan. Actually optimizing
> rule of JoinPushExpressionsRule is doing exactly the same kind of thing.
> Shall we just keep the optimization inside Optimized-Logical-Plan ? I mean
> shall we avoid calling *RelOptUtil.pushDownJoinConditions* in
> *SqlToRelConverter:createJoin*
> I raised this issue because that we are doing something based on the
> Logical-Plan. And it makes us really confused that the Logical-Plan doesn't
> corresponds to SqlNode.
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-2997) Avoid pushing down join condition in SqlToRelConverter
[
https://issues.apache.org/jira/browse/CALCITE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17111699#comment-17111699
]
Jin Xing commented on CALCITE-2997:
---
Thanks [~swtalbot] and [~julianhyde] to bring this up ~
Yes, In some scenarios we need SqlToRelConverter to return a logical plan which
corresponds to original SQL String exactly.
Mixing optimizations into the process of building logical plan sometimes make
the user confused.
I would +1 on Julian's idea – – move the rewrite/optimize code into RelBuilder
and add a config to enable/disable, but several concerns as below:
# Should we disable the rewrite/optimize in SqlToRelConverter by default ?
Calcite have clear phases for the lifecycle of a SQL. SqlToRelConverter can
return a purely reflection of the original SQL string, and let the
optimization/rewrite done in the following optimizing phase;
# In addition to pushDownJoinConditions, there are other rewrites like
pushDownNotForIn, shall we disable these optimizing work or even move out of
SqlToRelConverter to the optimizaing phase ?
> Avoid pushing down join condition in SqlToRelConverter
> --
>
> Key: CALCITE-2997
> URL: https://issues.apache.org/jira/browse/CALCITE-2997
> Project: Calcite
> Issue Type: Bug
>Reporter: Jin Xing
>Assignee: Julian Hyde
>Priority: Major
>
> In current code, *SqlToRelConverter:createJoin* is calling
> *RelOptUtil.pushDownJoinConditions* for optimization. And we can find below
> conversion from *SqlNode* to *RelNode*:
> {code:java}
> SqlNode:
> select * from A join B on A.x = B.x * 2
> RelNode (Logical-Plan):
> Join (condition:col0=col1)
> |-Project(x as col0)
> | |-Scan(A)
> |-Project(x * 2 as col1)
> |-Scan(B){code}
> As we can see the logical plan(*RelNode*) posted above is not the pure
> reflection of the original SQL String(*SqlNode*). The optimization is mixed
> into the phase on which AST is converted to Logical-Plan. Actually optimizing
> rule of JoinPushExpressionsRule is doing exactly the same kind of thing.
> Shall we just keep the optimization inside Optimized-Logical-Plan ? I mean
> shall we avoid calling *RelOptUtil.pushDownJoinConditions* in
> *SqlToRelConverter:createJoin*
> I raised this issue because that we are doing something based on the
> Logical-Plan. And it makes us really confused that the Logical-Plan doesn't
> corresponds to SqlNode.
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-2997) Avoid pushing down join condition in SqlToRelConverter
[
https://issues.apache.org/jira/browse/CALCITE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17111659#comment-17111659
]
Julian Hyde commented on CALCITE-2997:
--
Please review: https://github.com/julianhyde/calcite/tree/2997-join-push
> Avoid pushing down join condition in SqlToRelConverter
> --
>
> Key: CALCITE-2997
> URL: https://issues.apache.org/jira/browse/CALCITE-2997
> Project: Calcite
> Issue Type: Bug
>Reporter: Jin Xing
>Assignee: Julian Hyde
>Priority: Major
>
> In current code, *SqlToRelConverter:createJoin* is calling
> *RelOptUtil.pushDownJoinConditions* for optimization. And we can find below
> conversion from *SqlNode* to *RelNode*:
> {code:java}
> SqlNode:
> select * from A join B on A.x = B.x * 2
> RelNode (Logical-Plan):
> Join (condition:col0=col1)
> |-Project(x as col0)
> | |-Scan(A)
> |-Project(x * 2 as col1)
> |-Scan(B){code}
> As we can see the logical plan(*RelNode*) posted above is not the pure
> reflection of the original SQL String(*SqlNode*). The optimization is mixed
> into the phase on which AST is converted to Logical-Plan. Actually optimizing
> rule of JoinPushExpressionsRule is doing exactly the same kind of thing.
> Shall we just keep the optimization inside Optimized-Logical-Plan ? I mean
> shall we avoid calling *RelOptUtil.pushDownJoinConditions* in
> *SqlToRelConverter:createJoin*
> I raised this issue because that we are doing something based on the
> Logical-Plan. And it makes us really confused that the Logical-Plan doesn't
> corresponds to SqlNode.
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-2997) Avoid pushing down join condition in SqlToRelConverter
[
https://issues.apache.org/jira/browse/CALCITE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17106658#comment-17106658
]
Julian Hyde commented on CALCITE-2997:
--
I agree, [~swtalbot]. I have re-opened the issue. It should not have been
closed as 'invalid', because the requirement is a valid ask.
We're making ever more use of RelBuilder in SqlToRelConverter, and RelBuilder
performs various rewrites/optimizations as it goes. Some of these rewrites are
essential for the integrity of {{RelNode}}/{{RexNode}} data structure(e.g. the
assumption that {{(AND x (AND y z))}} is flattened to {{(AND x y z)}}), but
others are deemed "almost certainly beneficial".
This rewrite is of the latter category. We should make it possible to disable
such rewrites individually.
In this case (see
[code|https://github.com/apache/calcite/blob/e44beba286ea9049c5fd00c3a3b0e4a4f1c03356/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java#L2617]),
there is a {{RelBuilder}} involved but the rewrite is done by the
{{SqlToRelConverter}}. The ideal fix would move the rewrite code into
{{RelBuilder}}, and add a config parameter to {{RelBuilder}} so that people can
disable it. Less ideally, we would leave the code in {{SqlToRelConverter}} and
add a config parameter to {{SqlToRelConverter}}.
> Avoid pushing down join condition in SqlToRelConverter
> --
>
> Key: CALCITE-2997
> URL: https://issues.apache.org/jira/browse/CALCITE-2997
> Project: Calcite
> Issue Type: Bug
>Reporter: Jin Xing
>Assignee: Julian Hyde
>Priority: Major
>
> In current code, *SqlToRelConverter:createJoin* is calling
> *RelOptUtil.pushDownJoinConditions* for optimization. And we can find below
> conversion from *SqlNode* to *RelNode*:
> {code:java}
> SqlNode:
> select * from A join B on A.x = B.x * 2
> RelNode (Logical-Plan):
> Join (condition:col0=col1)
> |-Project(x as col0)
> | |-Scan(A)
> |-Project(x * 2 as col1)
> |-Scan(B){code}
> As we can see the logical plan(*RelNode*) posted above is not the pure
> reflection of the original SQL String(*SqlNode*). The optimization is mixed
> into the phase on which AST is converted to Logical-Plan. Actually optimizing
> rule of JoinPushExpressionsRule is doing exactly the same kind of thing.
> Shall we just keep the optimization inside Optimized-Logical-Plan ? I mean
> shall we avoid calling *RelOptUtil.pushDownJoinConditions* in
> *SqlToRelConverter:createJoin*
> I raised this issue because that we are doing something based on the
> Logical-Plan. And it makes us really confused that the Logical-Plan doesn't
> corresponds to SqlNode.
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-2997) Avoid pushing down join condition in SqlToRelConverter
[
https://issues.apache.org/jira/browse/CALCITE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17106592#comment-17106592
]
Steven Talbot commented on CALCITE-2997:
cc [~julianhyde]
I'd move to reopen this. Even if the original case perhaps does not need to
turn this off, I think there are valid cases where we would want to add a
config parameter to disable this behavior. While the push down provides a more
optimized plan, it also creates a more complex tree of RelNodes and writes much
more verbose SQL if those RelNodes are translated back to SQL with the Projects
that this rule creates. I could see a variety of cases where one would prefer
for those reasons to disable this behavior.
> Avoid pushing down join condition in SqlToRelConverter
> --
>
> Key: CALCITE-2997
> URL: https://issues.apache.org/jira/browse/CALCITE-2997
> Project: Calcite
> Issue Type: Bug
>Reporter: Jin Xing
>Priority: Major
>
> In current code, *SqlToRelConverter:createJoin* is calling
> *RelOptUtil.pushDownJoinConditions* for optimization. And we can find below
> conversion from *SqlNode* to *RelNode*:
> {code:java}
> SqlNode:
> select * from A join B on A.x = B.x * 2
> RelNode (Logical-Plan):
> Join (condition:col0=col1)
> |-Project(x as col0)
> | |-Scan(A)
> |-Project(x * 2 as col1)
> |-Scan(B){code}
> As we can see the logical plan(*RelNode*) posted above is not the pure
> reflection of the original SQL String(*SqlNode*). The optimization is mixed
> into the phase on which AST is converted to Logical-Plan. Actually optimizing
> rule of JoinPushExpressionsRule is doing exactly the same kind of thing.
> Shall we just keep the optimization inside Optimized-Logical-Plan ? I mean
> shall we avoid calling *RelOptUtil.pushDownJoinConditions* in
> *SqlToRelConverter:createJoin*
> I raised this issue because that we are doing something based on the
> Logical-Plan. And it makes us really confused that the Logical-Plan doesn't
> corresponds to SqlNode.
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (CALCITE-2997) Avoid pushing down join condition in SqlToRelConverter
[
https://issues.apache.org/jira/browse/CALCITE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16817584#comment-16817584
]
jin xing commented on CALCITE-2997:
---
THX a lot for comments ~Chunwei and ~Danny,
Our internal framework have some parsing rules on the RelNode pattern.
Yes~ For sure and no doubt that the logical plan is right. But for the concept
of parsing a SQL, there's no need to do the optimization redundantly in both
phase of AST->Logical-Plan and phase of Logical-Plan->Optimized-Logical-Plan.
I just try to ask that if there's any possibility to make the phase of
AST->Logical-Plan more clear.
> Avoid pushing down join condition in SqlToRelConverter
> --
>
> Key: CALCITE-2997
> URL: https://issues.apache.org/jira/browse/CALCITE-2997
> Project: Calcite
> Issue Type: Bug
>Reporter: jin xing
>Priority: Major
>
> In current code, *SqlToRelConverter:createJoin* is calling
> *RelOptUtil.pushDownJoinConditions* for optimization. And we can find below
> conversion from *SqlNode* to *RelNode*:
> {code:java}
> SqlNode:
> select * from A join B on A.x = B.x * 2
> RelNode (Logical-Plan):
> Join (condition:col0=col1)
> |-Project(x as col0)
> | |-Scan(A)
> |-Project(x * 2 as col1)
> |-Scan(B){code}
> As we can see the logical plan(*RelNode*) posted above is not the pure
> reflection of the original SQL String(*SqlNode*). The optimization is mixed
> into the phase on which AST is converted to Logical-Plan. Actually optimizing
> rule of JoinPushExpressionsRule is doing exactly the same kind of thing.
> Shall we just keep the optimization inside Optimized-Logical-Plan ? I mean
> shall we avoid calling *RelOptUtil.pushDownJoinConditions* in
> *SqlToRelConverter:createJoin*
> I raised this issue because that we are doing something based on the
> Logical-Plan. And it makes us really confused that the Logical-Plan doesn't
> corresponds to SqlNode.
>
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2997) Avoid pushing down join condition in SqlToRelConverter
[
https://issues.apache.org/jira/browse/CALCITE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16817500#comment-16817500
]
Danny Chan commented on CALCITE-2997:
-
So in what scene you must rely on the logical plan exactly ? Currently, sql to
rel conversion includes not only RelOptUtil#pushDownJoinConditions but also
RexNode simplification, decorrelation, some deterministic rewrite.
Maybe you should rely on the SqlNode tree but not RelNode.
> Avoid pushing down join condition in SqlToRelConverter
> --
>
> Key: CALCITE-2997
> URL: https://issues.apache.org/jira/browse/CALCITE-2997
> Project: Calcite
> Issue Type: Bug
>Reporter: jin xing
>Priority: Major
>
> In current code, *SqlToRelConverter:createJoin* is calling
> *RelOptUtil.pushDownJoinConditions* for optimization. And we can find below
> conversion from *SqlNode* to *RelNode*:
> {code:java}
> SqlNode:
> select * from A join B on A.x = B.x * 2
> RelNode (Logical-Plan):
> Join (condition:col0=col1)
> |-Project(x as col0)
> | |-Scan(A)
> |-Project(x * 2 as col1)
> |-Scan(B){code}
> As we can see the logical plan(*RelNode*) posted above is not the pure
> reflection of the original SQL String(*SqlNode*). The optimization is mixed
> into the phase on which AST is converted to Logical-Plan. Actually optimizing
> rule of JoinPushExpressionsRule is doing exactly the same kind of thing.
> Shall we just keep the optimization inside Optimized-Logical-Plan ? I mean
> shall we avoid calling *RelOptUtil.pushDownJoinConditions* in
> *SqlToRelConverter:createJoin*
> I raised this issue because that we are doing something based on the
> Logical-Plan. And it makes us really confused that the Logical-Plan doesn't
> corresponds to SqlNode.
>
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2997) Avoid pushing down join condition in SqlToRelConverter
[
https://issues.apache.org/jira/browse/CALCITE-2997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16817291#comment-16817291
]
Chunwei Lei commented on CALCITE-2997:
--
I believe that the logical plan is right anyway. Why do you want to make sure
logical plan is exactly corresponds to SqlNode?
> Avoid pushing down join condition in SqlToRelConverter
> --
>
> Key: CALCITE-2997
> URL: https://issues.apache.org/jira/browse/CALCITE-2997
> Project: Calcite
> Issue Type: Bug
>Reporter: jin xing
>Priority: Major
>
> In current code, *SqlToRelConverter:createJoin* is calling
> *RelOptUtil.pushDownJoinConditions* for optimization. And we can find below
> conversion from *SqlNode* to *RelNode*:
> {code:java}
> SqlNode:
> select * from A join B on A.x = B.x * 2
> RelNode (Logical-Plan):
> Join (condition:col0=col1)
> |-Project(x as col0)
> | |-Scan(A)
> |-Project(x * 2 as col1)
> |-Scan(B){code}
> As we can see the logical plan(*RelNode*) posted above is not the pure
> reflection of the original SQL String(*SqlNode*). The optimization is mixed
> into the phase on which AST is converted to Logical-Plan. Actually optimizing
> rule of JoinPushExpressionsRule is doing exactly the same kind of thing.
> Shall we just keep the optimization inside Optimized-Logical-Plan ? I mean
> shall we avoid calling *RelOptUtil.pushDownJoinConditions* in
> *SqlToRelConverter:createJoin*
> I raised this issue because that we are doing something based on the
> Logical-Plan. And it makes us really confused that the Logical-Plan doesn't
> corresponds to SqlNode.
>
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
