Re: copy method in RelSubset class

[Julian Hyde]

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 
>  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  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  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 inputs)
>> 
>> First of all, I don't understand the reason for the
>> *UnsupportedOperationException* in the first place. Why a RelSubset
>> shouldn't be copied?

Re: copy method in RelSubset class

[Alessandro Solimando]

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  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  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 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 

Re: copy method in RelSubset class

[Julian Hyde]

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 
>  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  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 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 

Re: copy method in RelSubset class

[Alessandro Solimando]

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  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 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 collations =
>> >>>rootRel.getTraitSet().getTraits(RelCollationTraitDef.INSTANCE);
>> >>>  rootRel = trimmer.trim(rootRel);
>> >>>  if (!ordered
>> >>>  && collations != null
>> >>>  && !collations.isEmpty()
>> >>>  && 

Re: copy method in RelSubset class

[Alessandro Solimando]

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  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 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 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 

Re: copy method in RelSubset class

[Julian Hyde]

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 
>  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 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 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(CalciteAssert.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.returnsUnordered(CalciteAssert.java:1357)
>> 
>> at
>>> org.apache.calcite.test.SparkAdapterTest.commonTester(SparkAdapterTest.java:93)
>> 
>> at
>>> org.apache.calcite.test.SparkAdapterTest.testFilterBetween(SparkAdapterTest.java:460)
>> 
>> 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
>>>