Yes, it’s usually better that a bug as is described in terms of behavior an end-user would see, and has a test case for that behavior, rather than talking about internals “method x should call method y”.
> On Feb 16, 2018, at 11:42 AM, Alessandro Solimando > <alessandro.solima...@gmail.com> wrote: > > 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 <jh...@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.solima...@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.solima...@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 <jh...@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.solima...@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 >>>>>>> >>>>>>> >>>>> >>>>> >>>> >> >>
