calcite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alessandro Solimando <alessandro.solima...@gmail.com>
Subject Re: copy method in RelSubset class
Date Fri, 16 Feb 2018 19:42:48 GMT
Thanks Julian, the ticket is open:
https://issues.apache.org/jira/browse/CALCITE-2184

Given its size, I plan to propose the "fix" along with the PR with the new
Spark adapter tests if that's ok.

On 15 February 2018 at 19:46, Julian Hyde <jhyde@apache.org> wrote:

> Yes, log a JIRA case. Making RelSubset.copy return this, rather than
> throw, seems pragmatic.
>
> Long-term I would like to get rid of copy, so we might reverse this change
> at some point. But by that time, these tests will be enabled.
>
> Julian
>
>
> > On Feb 14, 2018, at 4:04 PM, Alessandro Solimando <
> alessandro.solimando@gmail.com> wrote:
> >
> > Hi,
> > while preparing some additional unit tests for the spark adapter (
> > https://github.com/asolimando/calcite/tree/SPARK-TESTS) I have stumbled
> > upon the issue several times.
> >
> > This is the list of the tests that in my opinion should be succeeding but
> > are failing because of an invocation of *copy* method for *RelSubset*
> class:
> > - testFilterBetween
> > - testFilterIsIn
> > - testFilterTrue
> > - testFilterFalse
> > - testFilterOr
> > - testFilterIsNotNull
> > - testFilterIsNull
> >
> > As you can infer from the name, the common trait among them is the
> presence
> > of a "complex" filtering condition in the where clause.
> >
> > Just as a side note (not implying this is a solution), by replacing the
> > exception throwing with "return this;" inside *RelSubset.copy, *the
> > aforementioned tests pass.
> >
> > Can you please acknowledge the issue (if any) so I can open a ticket, and
> > reference it in the "@Ignore" of those tests, so I can advance with the
> PR?
> >
> > Best regards,
> > Alessandro
> >
> > On 12 February 2018 at 09:56, Alessandro Solimando <
> > alessandro.solimando@gmail.com> wrote:
> >
> >> Hello Julian,
> >> If I got it right, trimming the query plan for unused fields is a
> top-down
> >> procedure removing any reference to unused fields in the subplan rooted
> at
> >> the considered tree node.
> >>
> >> This, in principle, can affect also those coming from elements of
> >> *RelSubset*, independently from the fact that they are in an equivalence
> >> class, and that their result is "immutable". The only source of problem
> I
> >> see, is that the very concept of *RelSubset* suggests a "global scope",
> >> and updating it according to the contextual information of a specific
> >> subplan could break its correctness (the relational expressions
> composing
> >> *RelSubset* would be fitting only some of original contexts in which
> they
> >> were originally equivalent).
> >>
> >> However, *trimUnusedFields*, in my example, tries to update the traits
> of
> >> RelSubset's elements.
> >>
> >> So, if *RelSubset* should be immutable (traits included), then the
> >> *trimUnusedFields* method should never call *copy* on it, but it does,
> >> and the exception is thrown.
> >>
> >> The fact that implementing copy for *RelSubset* as the identity (that
> is,
> >> simply returning "this", ignoring any modification to the traits) did
> not
> >> introduce any problem reinforces the immutability hypothesis.
> >>
> >> Is my understanding correct?
> >> Given that the query looks legal, the problem looks "real".
> >> If this is confirmed, how do you suggest to address it?
> >>
> >> On 12 February 2018 at 00:04, Julian Hyde <jhyde@apache.org> wrote:
> >>
> >>> Can you tell me why you want to copy a RelSubset?
> >>>
> >>> A RelSubset is an equivalence class - a set of relational expressions
> >>> that always return the same results. So if you made a copy you’d be
> >>> creating another equivalent relational expression - that by definition
> >>> should be in the original RelSubset.
> >>>
> >>>> On Feb 11, 2018, at 1:18 PM, Alessandro Solimando <
> >>> alessandro.solimando@gmail.com> wrote:
> >>>>
> >>>> Hello community,
> >>>>
> >>>> I am adding a SparkAdapter test with the following query:
> >>>>
> >>>>> select *
> >>>>> from *(values (1, 'a'), (2, 'b'), (3, 'b'), (4, 'c'), (2, 'c'))
as
> >>> t(x, y)*
> >>>>> where x between 3 and 4
> >>>>>
> >>>>> When executed, an exception is thrown (the full stack trace is at
the
> >>> end
> >>>> of the email) in *copy* method in *RelSubset* class, while Calcite is
> >>>> trying to get rid of unused terms (specifically, *trimUnusedFields*
> >>> method
> >>>> from *SqlToRelConverted* class).
> >>>>
> >>>> The signature of copy is as follows: public RelNode copy(RelTraitSet
> >>>> traitSet, List<RelNode> inputs)
> >>>>
> >>>> First of all, I don't understand the reason for the
> >>>> *UnsupportedOperationException* in the first place. Why a RelSubset
> >>>> shouldn't be copied?
> >>>>
> >>>> Assuming that the functionality is simply missing, I have considered
> two
> >>>> alternatives for implementing it:
> >>>> 1) copy as the identity function -> all Calcite tests pass, but I
am
> >>>> ignoring the *traitSet* parameter in this way, looks odd
> >>>> 2) I have tried to build a new *RelSubset* by reusing the cluster and
> >>> set
> >>>> information from the object, and the trait argument of copy -> assert
> >>>> traits.allSimple();
> >>>> fails in the constructor
> >>>>
> >>>> In my example, the trait "[1]" (ordering detected at tuple level on
> the
> >>> 2nd
> >>>> component) is transformed into a composite trait "[[1]]", this makes
> the
> >>>> assertion fail.
> >>>> While I know what a trait is, I don't understand what a composite one
> >>> is.
> >>>> Do you have a concrete example?
> >>>>
> >>>> So the problem here is the introduction of the composite trait, which
> is
> >>>> caused by the *replace* method in *trimUnusedFields*:
> >>>>
> >>>> if (isTrimUnusedFields()) {
> >>>>>> final RelFieldTrimmer trimmer = newFieldTrimmer();
> >>>>>> final List<RelCollation> collations =
> >>>>>>   rootRel.getTraitSet().getTraits(RelCollationTraitDef.INSTANCE);
> >>>>>> rootRel = trimmer.trim(rootRel);
> >>>>>> if (!ordered
> >>>>>> && collations != null
> >>>>>> && !collations.isEmpty()
> >>>>>> && !collations.equals(ImmutableList.of(RelCollations.EMPTY)))
{
> >>>>>>   final RelTraitSet traitSet = rootRel.getTraitSet()
> >>>>>>     .replace(RelCollationTraitDef.INSTANCE, collations);
> >>>>>>   rootRel = rootRel.copy(traitSet, rootRel.getInputs());
> >>>>>> }
> >>>>>> if (SQL2REL_LOGGER.isDebugEnabled()) {
> >>>>>>   SQL2REL_LOGGER.debug(
> >>>>>>     RelOptUtil.dumpPlan("Plan after trimming unused fields",
> rootRel,
> >>>>>>     SqlExplainFormat.TEXT, SqlExplainLevel.EXPPLAN_ATTRIBUTES));
> >>>>>>  }
> >>>>>> }
> >>>>>
> >>>>>
> >>>> It is also not clear to me what the first *if* is trying to accomplish
> >>>> here. I mean, the traits are never modified here, so it really looks
> >>> like
> >>>> the only reason for calling *replace* is to apply whatever side effect
> >>> this
> >>>> method has (the conversion from traits to composite traits looks the
> >>> only
> >>>> one to me), and in the specific situations matching the if condition.
> >>> Can
> >>>> you clarify which scenario the if is handling?
> >>>>
> >>>> I would appreciate also a feedback on the implementation of copy as
> >>>> identity. Is it correct for you?
> >>>> Or do you suggest the second option by enforcing a flattening of
> traits
> >>>> before calling the constructor?
> >>>>
> >>>> Best regards,
> >>>> Alessandro
> >>>>
> >>>> The full stack trace:
> >>>>
> >>>> java.lang.RuntimeException: With materializationsEnabled=false,
> limit=0
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.CalciteAssert.assertQuery(CalciteAss
> >>> ert.java:600)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.CalciteAssert$AssertQuery.returns(
> >>> CalciteAssert.java:1346)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.CalciteAssert$AssertQuery.returns(
> >>> CalciteAssert.java:1329)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.CalciteAssert$AssertQuery.returnsUno
> >>> rdered(CalciteAssert.java:1357)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.SparkAdapterTest.commonTester(SparkA
> >>> dapterTest.java:93)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.SparkAdapterTest.testFilterBetween(S
> >>> parkAdapterTest.java:460)
> >>>>>
> >>>>> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> >>>>>
> >>>>> at
> >>>>>> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAcce
> >>> ssorImpl.java:62)
> >>>>>
> >>>>> at
> >>>>>> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMe
> >>> thodAccessorImpl.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(Refl
> >>> ectiveCallable.java:12)
> >>>>>
> >>>>> at
> >>>>>> org.junit.runners.model.FrameworkMethod.invokeExplosively(Fr
> >>> ameworkMethod.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(BlockJUnit
> >>> 4ClassRunner.java:78)
> >>>>>
> >>>>> at
> >>>>>> org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit
> >>> 4ClassRunner.java:57)
> >>>>>
> >>>>> at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
> >>>>>
> >>>>> at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
> >>>>>
> >>>>> at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
> >>>>>
> >>>>> at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
> >>>>>
> >>>>> at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
> >>>>>
> >>>>> at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
> >>>>>
> >>>>> at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
> >>>>>
> >>>>> at
> >>>>>> com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs
> >>> (JUnit4IdeaTestRunner.java:68)
> >>>>>
> >>>>> at
> >>>>>> com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.star
> >>> tRunnerWithArgs(IdeaTestRunner.java:47)
> >>>>>
> >>>>> at
> >>>>>> com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsA
> >>> ndStart(JUnitStarter.java:242)
> >>>>>
> >>>>> at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStart
> >>> er.java:70)
> >>>>>
> >>>>> Caused by: java.sql.SQLException: Error while executing SQL "select
*
> >>>>>
> >>>>> from (values (1, 'a'), (2, 'b'), (3, 'b'), (4, 'c'), (2, 'c')) as
> t(x,
> >>> y)
> >>>>>
> >>>>> where x between 3 and 4": null
> >>>>>
> >>>>> at org.apache.calcite.avatica.Helper.createException(Helper.java:56)
> >>>>>
> >>>>> at org.apache.calcite.avatica.Helper.createException(Helper.java:41)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.avatica.AvaticaStatement.executeInternal(
> >>> AvaticaStatement.java:156)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.avatica.AvaticaStatement.executeQuery(Ava
> >>> ticaStatement.java:218)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.CalciteAssert.assertQuery(CalciteAss
> >>> ert.java:568)
> >>>>>
> >>>>> ... 27 more
> >>>>>
> >>>>> Caused by: java.lang.UnsupportedOperationException
> >>>>>
> >>>>> at org.apache.calcite.plan.volcano.RelSubset.copy(
> RelSubset.java:149)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.sql2rel.SqlToRelConverter.trimUnusedField
> >>> s(SqlToRelConverter.java:517)
> >>>>>
> >>>>> at org.apache.calcite.prepare.Prepare.trimUnusedFields(Prepare.
> >>> java:391)
> >>>>>
> >>>>> at org.apache.calcite.prepare.Prepare.prepareSql(Prepare.java:304)
> >>>>>
> >>>>> at org.apache.calcite.prepare.Prepare.prepareSql(Prepare.java:230)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.prepare.CalcitePrepareImpl.prepare2_(Calc
> >>> itePrepareImpl.java:781)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.prepare.CalcitePrepareImpl.prepare_(Calci
> >>> tePrepareImpl.java:640)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.prepare.CalcitePrepareImpl.prepareSql(Cal
> >>> citePrepareImpl.java:610)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.jdbc.CalciteConnectionImpl.parseQuery(Cal
> >>> citeConnectionImpl.java:221)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.jdbc.CalciteMetaImpl.prepareAndExecute(Ca
> >>> lciteMetaImpl.java:603)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.avatica.AvaticaConnection.prepareAndExecu
> >>> teInternal(AvaticaConnection.java:638)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.avatica.AvaticaStatement.executeInternal(
> >>> AvaticaStatement.java:149)
> >>>>>
> >>>>> ... 29 more
> >>>>>
> >>>>>
> >>>>>> java.lang.RuntimeException: exception while executing [select
*
> >>>>>
> >>>>> from (values (1, 'a'), (2, 'b'), (3, 'b'), (4, 'c'), (2, 'c')) as
> t(x,
> >>> y)
> >>>>>
> >>>>> where x between 3 and 4]
> >>>>>
> >>>>>
> >>>>>> at
> >>>>>> org.apache.calcite.test.CalciteAssert$AssertQuery.returns(
> >>> CalciteAssert.java:1351)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.CalciteAssert$AssertQuery.returns(
> >>> CalciteAssert.java:1329)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.CalciteAssert$AssertQuery.returnsUno
> >>> rdered(CalciteAssert.java:1357)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.SparkAdapterTest.commonTester(SparkA
> >>> dapterTest.java:93)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.SparkAdapterTest.testFilterBetween(S
> >>> parkAdapterTest.java:460)
> >>>>>
> >>>>> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> >>>>>
> >>>>> at
> >>>>>> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAcce
> >>> ssorImpl.java:62)
> >>>>>
> >>>>> at
> >>>>>> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMe
> >>> thodAccessorImpl.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(Refl
> >>> ectiveCallable.java:12)
> >>>>>
> >>>>> at
> >>>>>> org.junit.runners.model.FrameworkMethod.invokeExplosively(Fr
> >>> ameworkMethod.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(BlockJUnit
> >>> 4ClassRunner.java:78)
> >>>>>
> >>>>> at
> >>>>>> org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit
> >>> 4ClassRunner.java:57)
> >>>>>
> >>>>> at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
> >>>>>
> >>>>> at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
> >>>>>
> >>>>> at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
> >>>>>
> >>>>> at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
> >>>>>
> >>>>> at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
> >>>>>
> >>>>> at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
> >>>>>
> >>>>> at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
> >>>>>
> >>>>> at
> >>>>>> com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs
> >>> (JUnit4IdeaTestRunner.java:68)
> >>>>>
> >>>>> at
> >>>>>> com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.star
> >>> tRunnerWithArgs(IdeaTestRunner.java:47)
> >>>>>
> >>>>> at
> >>>>>> com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsA
> >>> ndStart(JUnitStarter.java:242)
> >>>>>
> >>>>> at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStart
> >>> er.java:70)
> >>>>>
> >>>>> Caused by: java.lang.RuntimeException: With
> >>> materializationsEnabled=false,
> >>>>>> limit=0
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.CalciteAssert.assertQuery(CalciteAss
> >>> ert.java:600)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.CalciteAssert$AssertQuery.returns(
> >>> CalciteAssert.java:1346)
> >>>>>
> >>>>> ... 26 more
> >>>>>
> >>>>> Caused by: java.sql.SQLException: Error while executing SQL "select
*
> >>>>>
> >>>>> from (values (1, 'a'), (2, 'b'), (3, 'b'), (4, 'c'), (2, 'c')) as
> t(x,
> >>> y)
> >>>>>
> >>>>> where x between 3 and 4": null
> >>>>>
> >>>>> at org.apache.calcite.avatica.Helper.createException(Helper.java:56)
> >>>>>
> >>>>> at org.apache.calcite.avatica.Helper.createException(Helper.java:41)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.avatica.AvaticaStatement.executeInternal(
> >>> AvaticaStatement.java:156)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.avatica.AvaticaStatement.executeQuery(Ava
> >>> ticaStatement.java:218)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.test.CalciteAssert.assertQuery(CalciteAss
> >>> ert.java:568)
> >>>>>
> >>>>> ... 27 more
> >>>>>
> >>>>> Caused by: java.lang.UnsupportedOperationException
> >>>>>
> >>>>> at org.apache.calcite.plan.volcano.RelSubset.copy(
> RelSubset.java:149)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.sql2rel.SqlToRelConverter.trimUnusedField
> >>> s(SqlToRelConverter.java:517)
> >>>>>
> >>>>> at org.apache.calcite.prepare.Prepare.trimUnusedFields(Prepare.
> >>> java:391)
> >>>>>
> >>>>> at org.apache.calcite.prepare.Prepare.prepareSql(Prepare.java:304)
> >>>>>
> >>>>> at org.apache.calcite.prepare.Prepare.prepareSql(Prepare.java:230)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.prepare.CalcitePrepareImpl.prepare2_(Calc
> >>> itePrepareImpl.java:781)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.prepare.CalcitePrepareImpl.prepare_(Calci
> >>> tePrepareImpl.java:640)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.prepare.CalcitePrepareImpl.prepareSql(Cal
> >>> citePrepareImpl.java:610)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.jdbc.CalciteConnectionImpl.parseQuery(Cal
> >>> citeConnectionImpl.java:221)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.jdbc.CalciteMetaImpl.prepareAndExecute(Ca
> >>> lciteMetaImpl.java:603)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.avatica.AvaticaConnection.prepareAndExecu
> >>> teInternal(AvaticaConnection.java:638)
> >>>>>
> >>>>> at
> >>>>>> org.apache.calcite.avatica.AvaticaStatement.executeInternal(
> >>> AvaticaStatement.java:149)
> >>>>>
> >>>>> ... 29 more
> >>>>>
> >>>>>
> >>>
> >>>
> >>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message