[
https://issues.apache.org/jira/browse/CALCITE-2667?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16694642#comment-16694642
]
Piotr Bojko edited comment on CALCITE-2667 at 11/21/18 12:19 PM:
-----------------------------------------------------------------
I have found two solutions
*Solution 1*
This one is based on assumption that SqlToRelConverter.leaves are out of sync
with a relational tree when RelBuilder pushes down join conditions
(RelOptUtil.pushDownJoinConditions). In such case left or/and right inputs of a
join could be merged into new projects.
The solution was to resync SqlToRelConverter.leaves with a new merged project.
But with this approach next problem occured. As in my previous comment -
SqlToRelConverter.convertIdentifier assumes that looking an identifier through
inputs based on an offset is the same as looking mentioned through union of
inputs and leaves. Such assumption is not correct when in a relational tree
would be a project with permutation in its expressions - such case can occur
when new merged projects is synced! I have changed this logic a little bit -
now identifier is being search through union of inputs and leaves and projected
on to inputs (offset is being recalculated to match the proper coordinates in
the inputs).
However I've come to conclusion that my implementation (the second part) is a
little bit naive and it takes only RexInputRefs into consideration when
recalculated offset for inputs. Such prototype I've pushed into
https://github.com/ptrbojko/calcite/tree/bug/CALCITE-2667 with an example of
one failing tests showing RexInputRefs are not enough.
*Side note* - I think that SqlToRelConverter.convertIdentifier is very fragile
because of the mentioned assumption in its logic. In my honest opinion it
should be rewritten, maybe as the proposed approach.
See https://github.com/ptrbojko/calcite/tree/bug/CALCITE-2667 for details.
*Solution 2*
This one is based on assumption that we should prevent merging projects in
RelOptUtil.pushDownJoinConditions when merged projects is a leaf in
SqlToRelConverter. I've extended then RelBuilder to be configurable and
SqlToRelConverter enriching RelBuilder with a check if a project is not a leaf
and can be merged.
This solution alters expected relational tree. I had to tune some tests which
were checking expected plans, because of missing merge.
Pull request is here - https://github.com/apache/calcite/pull/931
*Side note*: I've choose the second approach because it does not alter any
assumptions and was less invasive.
was (Author: ptrbojko):
I have found two solutions
*Solution 1*
This one is based on assumption that SqlToRelConverter.leaves are out of sync
with a relational tree when RelBuilder pushes down join conditions
(RelOptUtil.pushDownJoinConditions). In such case left or/and right inputs of a
join could be merged into new projects.
The solution was to resync SqlToRelConverter.leaves with a new merged project.
But with this approach next problem occured. As in my previous comment -
SqlToRelConverter.convertIdentifier assumes that looking an identifier through
inputs based on an offset is the same as looking mentioned through union of
inputs and leaves. Such assumption is not correct when in a relational tree
would be a project with permutation in its expressions - such case can occur
when new merged projects is synced! I have changed this logic a little bit -
now identifier is being search through union of inputs and leaves and projected
on to inputs (offset is being recalculated to match the proper coordinates in
the inputs).
However I've come to conclusion that my implementation (the second part) is a
little bit naive and it takes only RexInputRefs into consideration when
recalculated offset for inputs. Such prototype I've pushed into
https://github.com/ptrbojko/calcite/tree/bug/CALCITE-2667 with an example of
one failing tests showing RexInputRefs are not enough.
*Side note* - I think that SqlToRelConverter.convertIdentifier is very fragile
because of the mentioned assumption in its logic. In my honest opinion it
should be rewritten, maybe as the proposed approach.
See https://github.com/ptrbojko/calcite/tree/bug/CALCITE-2667 for details.
*Solution 2*
This one is based on assumption that we should prevent merging projects in
RelOptUtil.pushDownJoinConditions when merged projects is a leaf in
SqlToRelConverter. I've extended then RelBuilder to be configurable and
SqlToRelConverter enriching RelBuilder with a check if a project is not a leaf
and can be merged.
This solution alters expected relational tree. I had to tune some tests which
were checking expected plans, because of missing merge.
Pull request is here - https://github.com/apache/calcite/pull/931
> sql2rel fails on join between table function and other table
> ------------------------------------------------------------
>
> Key: CALCITE-2667
> URL: https://issues.apache.org/jira/browse/CALCITE-2667
> Project: Calcite
> Issue Type: Bug
> Reporter: Piotr Bojko
> Assignee: Julian Hyde
> Priority: Major
>
> Bug reproduction is here -
> https://github.com/ptrbojko/calcite/tree/bug/CALCITE-2667
> {noformat}
> > java.lang.AssertionError: Field ordinal 2 is invalid for type
> > 'RecordType(VARCHAR TYPE, VARCHAR CODEVALUE)'
> > at
> > org.apache.calcite.rex.RexBuilder.makeFieldAccess(RexBuilder.java:188)
> > at
> > org.apache.calcite.sql2rel.SqlToRelConverter.convertIdentifier(SqlToRelConverter.java:3616)
> > at
> > org.apache.calcite.sql2rel.SqlToRelConverter.access$2100(SqlToRelConverter.java:215)
> > at
> > org.apache.calcite.sql2rel.SqlToRelConverter$Blackboard.visit(SqlToRelConverter.java:4691)
> > at
> > org.apache.calcite.sql2rel.SqlToRelConverter$Blackboard.visit(SqlToRelConverter.java:3987)
> > at org.apache.calcite.sql.SqlIdentifier.accept(SqlIdentifier.java:334)
> > at
> > org.apache.calcite.sql2rel.SqlToRelConverter$Blackboard.convertExpression(SqlToRelConverter.java:4551)
> > at
> > org.apache.calcite.sql2rel.StandardConvertletTable.lambda$new$9(StandardConvertletTable.java:204)
> > at
> > org.apache.calcite.sql2rel.SqlNodeToRexConverterImpl.convertCall(SqlNodeToRexConverterImpl.java:63)
> > at
> > org.apache.calcite.sql2rel.SqlToRelConverter$Blackboard.visit(SqlToRelConverter.java:4682)
> > at
> > org.apache.calcite.sql2rel.SqlToRelConverter$Blackboard.visit(SqlToRelConverter.java:3987)
> > at org.apache.calcite.sql.SqlCall.accept(SqlCall.java:138)
> > at
> > org.apache.calcite.sql2rel.SqlToRelConverter$Blackboard.convertExpression(SqlToRelConverter.java:4551)
> > at
> > org.apache.calcite.sql2rel.SqlToRelConverter.convertSelectList(SqlToRelConverter.java:3830)
> > at
> > org.apache.calcite.sql2rel.SqlToRelConverter.convertSelectImpl(SqlToRelConverter.java:668)
> > at
> > org.apache.calcite.sql2rel.SqlToRelConverter.convertSelect(SqlToRelConverter.java:625)
> > at
> > org.apache.calcite.sql2rel.SqlToRelConverter.convertQueryRecursive(SqlToRelConverter.java:3074)
> > at
> > org.apache.calcite.sql2rel.SqlToRelConverter.convertQuery(SqlToRelConverter.java:561)
> > at org.apache.calcite.prepare.Prepare.prepareSql(Prepare.java:265)
> > at org.apache.calcite.prepare.Prepare.prepareSql(Prepare.java:231)
> > at
> > org.apache.calcite.prepare.CalcitePrepareImpl.prepare2_(CalcitePrepareImpl.java:772)
> > at
> > org.apache.calcite.prepare.CalcitePrepareImpl.prepare_(CalcitePrepareImpl.java:636)
> > at
> > org.apache.calcite.prepare.CalcitePrepareImpl.prepareSql(CalcitePrepareImpl.java:606)
> > at
> > org.apache.calcite.jdbc.CalciteConnectionImpl.parseQuery(CalciteConnectionImpl.java:229)
> > at
> > org.apache.calcite.jdbc.CalciteMetaImpl.prepareAndExecute(CalciteMetaImpl.java:550)
> > at
> > org.apache.calcite.avatica.AvaticaConnection.prepareAndExecuteInternal(AvaticaConnection.java:675)
> > at
> > org.apache.calcite.avatica.AvaticaStatement.executeInternal(AvaticaStatement.java:156)
> > at
> > org.apache.calcite.avatica.AvaticaStatement.executeQuery(AvaticaStatement.java:227)
> > at net.hydromatic.quidem.Quidem.checkResult(Quidem.java:322)
> > at net.hydromatic.quidem.Quidem.access$2800(Quidem.java:54)
> > at
> > net.hydromatic.quidem.Quidem$ContextImpl.checkResult(Quidem.java:1747)
> > at
> > net.hydromatic.quidem.Quidem$CheckResultCommand.execute(Quidem.java:1078)
> > at
> > net.hydromatic.quidem.Quidem$CompositeCommand.execute(Quidem.java:1548)
> > at net.hydromatic.quidem.Quidem.execute(Quidem.java:216)
> > at org.apache.calcite.test.QuidemTest.checkRun(QuidemTest.java:161)
> > at org.apache.calcite.test.QuidemTest.test(QuidemTest.java:209)
> > at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> > at
> > sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> > at
> > sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> > at java.lang.reflect.Method.invoke(Method.java:498)
> > at
> > org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
> > at
> > org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
> > at
> > org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
> > at
> > org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
> > at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
> > at
> > org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
> > at
> > org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
> > at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
> > at
> > org.apache.maven.surefire.junitcore.pc.Scheduler$1.run(Scheduler.java:410)
> > at
> > java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
> > at java.util.concurrent.FutureTask.run(FutureTask.java:266)
> > at
> > java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
> > at
> > java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
> > at java.lang.Thread.run(Thread.java:748)
> {noformat}
> I think it worked pretty ok on CALCITE-1.17 version
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
