Re: RelOptRulesTest.java and RelOptRulesTest.xml

[Benchao Li]

Thanks Alessandro for driving this.

It's great to hear this is being improved. I suffered from this problem
several times before.

Alessandro Solimando  于2022年10月24日周一
17:25写道：

> I have a working implementation in the PR 2948
>  for CALCITE-5340
> , it's covering the
> following test suites:
>
> - HepPlannerTest
> - RelOptRulesTest
> - SqlHintsConverterTest
> - SqlToRelConverterTest
> - TypeCoercionConverterTest
>
> The associated XML reference files will be updated in the context of this
> PR, and from now on the tests will fail upon discrepancies.
>
> Happy to hear your opinion if you have time.
>
> Best regards,
> Alessandro
>
> On Fri, 21 Oct 2022 at 10:42, Alessandro Solimando <
> alessandro.solima...@gmail.com> wrote:
>
> > Thank you guys for your input.
> >
> > Given that the official way to update the XML file (as per our doc) is to
> > automatically generate it, I think it should fail if the actual and
> > expected files differ, whatever the difference is.
> >
> > I will dig more into how DiffRepository works and make the test fail on
> > discrepancies in a subsequent PR.
> >
> > In the meantime, I have filed CALCITE-5340
> >  to track this.
> >
> > Best regards,
> > Alessandro
> >
> > On Fri, 21 Oct 2022 at 10:16, Stamatis Zampetakis 
> > wrote:
> >
> >> Thanks for taking care of this Alessandro!
> >>
> >> I believe the resources are still sorted as per [1] but it appears that
> >> there are still a few manual edits to the file.
> >>
> >> If we want to prevent such changes in the future it may be a good idea
> to
> >> add a post-class check In RelOptRulesTest (or somewhere in DiffRepo)
> >> checking that files are identical.
> >>
> >> Best,
> >> Stamatis
> >>
> >> [1] https://lists.apache.org/thread/j0fh9hhxdc9lc7n228kwj6xcs0z9hpb3
> >>
> >> On Thu, Oct 20, 2022 at 8:36 PM Julian Hyde 
> >> wrote:
> >>
> >> > Do you think that RelOptRulesTest should fail if the resources are not
> >> > sorted alphabetically? (I thought it did that at some point… I may be
> >> > wrong. Anyway, I am a fan of ‘fail fast’. The person who introduces
> the
> >> > error should be the person to fix it.)
> >> >
> >> > > On Oct 20, 2022, at 9:20 AM, Alessandro Solimando <
> >> > alessandro.solima...@gmail.com> wrote:
> >> > >
> >> > > Hello everyone,
> >> > > I have recently added tests to RelOptRulesTest.java, took the chance
> >> to
> >> > > update (maven instruction with gradle instructions, path has
> changed)
> >> and
> >> > > improved them a little bit.
> >> > >
> >> > > While testing the commands I have noticed that few PRs in the last
> >> months
> >> > > had tests added to RelOptRulesTest.java with a manual update of
> >> > > RelOptRulesTest.xml (possibly due to the stale instructions).
> >> > >
> >> > > Manual updates often suffer from formatting issues, stale/wrong SQL
> >> > > statements, different test order.
> >> > >
> >> > > Note that those discrepancies are not caught by running tests/CI,
> but
> >> > it's
> >> > > nonetheless problematic, especially because it forbids folks down
> the
> >> > line
> >> > > to re-generate the file automatically, because their PRs would carry
> >> the
> >> > > changes from past work.
> >> > >
> >> > > In https://github.com/apache/calcite/pull/2944 I will put back on
> >> track
> >> > > RelOptRulesTest.xml, please try to follow the procedure in the
> >> javadoc of
> >> > > RelOptRulesTest.java and let me know if we can improve them even
> >> further
> >> > in
> >> > > your opinion.
> >> > >
> >> > > Best regards,
> >> > > Alessandro
> >> >
> >> >
> >>
> >
>


-- 

Best,
Benchao Li

Re: RelOptRulesTest.java and RelOptRulesTest.xml

[Alessandro Solimando]

I have a working implementation in the PR 2948
 for CALCITE-5340
, it's covering the
following test suites:

- HepPlannerTest
- RelOptRulesTest
- SqlHintsConverterTest
- SqlToRelConverterTest
- TypeCoercionConverterTest

The associated XML reference files will be updated in the context of this
PR, and from now on the tests will fail upon discrepancies.

Happy to hear your opinion if you have time.

Best regards,
Alessandro

On Fri, 21 Oct 2022 at 10:42, Alessandro Solimando <
alessandro.solima...@gmail.com> wrote:

> Thank you guys for your input.
>
> Given that the official way to update the XML file (as per our doc) is to
> automatically generate it, I think it should fail if the actual and
> expected files differ, whatever the difference is.
>
> I will dig more into how DiffRepository works and make the test fail on
> discrepancies in a subsequent PR.
>
> In the meantime, I have filed CALCITE-5340
>  to track this.
>
> Best regards,
> Alessandro
>
> On Fri, 21 Oct 2022 at 10:16, Stamatis Zampetakis 
> wrote:
>
>> Thanks for taking care of this Alessandro!
>>
>> I believe the resources are still sorted as per [1] but it appears that
>> there are still a few manual edits to the file.
>>
>> If we want to prevent such changes in the future it may be a good idea to
>> add a post-class check In RelOptRulesTest (or somewhere in DiffRepo)
>> checking that files are identical.
>>
>> Best,
>> Stamatis
>>
>> [1] https://lists.apache.org/thread/j0fh9hhxdc9lc7n228kwj6xcs0z9hpb3
>>
>> On Thu, Oct 20, 2022 at 8:36 PM Julian Hyde 
>> wrote:
>>
>> > Do you think that RelOptRulesTest should fail if the resources are not
>> > sorted alphabetically? (I thought it did that at some point… I may be
>> > wrong. Anyway, I am a fan of ‘fail fast’. The person who introduces the
>> > error should be the person to fix it.)
>> >
>> > > On Oct 20, 2022, at 9:20 AM, Alessandro Solimando <
>> > alessandro.solima...@gmail.com> wrote:
>> > >
>> > > Hello everyone,
>> > > I have recently added tests to RelOptRulesTest.java, took the chance
>> to
>> > > update (maven instruction with gradle instructions, path has changed)
>> and
>> > > improved them a little bit.
>> > >
>> > > While testing the commands I have noticed that few PRs in the last
>> months
>> > > had tests added to RelOptRulesTest.java with a manual update of
>> > > RelOptRulesTest.xml (possibly due to the stale instructions).
>> > >
>> > > Manual updates often suffer from formatting issues, stale/wrong SQL
>> > > statements, different test order.
>> > >
>> > > Note that those discrepancies are not caught by running tests/CI, but
>> > it's
>> > > nonetheless problematic, especially because it forbids folks down the
>> > line
>> > > to re-generate the file automatically, because their PRs would carry
>> the
>> > > changes from past work.
>> > >
>> > > In https://github.com/apache/calcite/pull/2944 I will put back on
>> track
>> > > RelOptRulesTest.xml, please try to follow the procedure in the
>> javadoc of
>> > > RelOptRulesTest.java and let me know if we can improve them even
>> further
>> > in
>> > > your opinion.
>> > >
>> > > Best regards,
>> > > Alessandro
>> >
>> >
>>
>

Re: RelOptRulesTest.java and RelOptRulesTest.xml

[Alessandro Solimando]

Thank you guys for your input.

Given that the official way to update the XML file (as per our doc) is to
automatically generate it, I think it should fail if the actual and
expected files differ, whatever the difference is.

I will dig more into how DiffRepository works and make the test fail on
discrepancies in a subsequent PR.

In the meantime, I have filed CALCITE-5340
 to track this.

Best regards,
Alessandro

On Fri, 21 Oct 2022 at 10:16, Stamatis Zampetakis  wrote:

> Thanks for taking care of this Alessandro!
>
> I believe the resources are still sorted as per [1] but it appears that
> there are still a few manual edits to the file.
>
> If we want to prevent such changes in the future it may be a good idea to
> add a post-class check In RelOptRulesTest (or somewhere in DiffRepo)
> checking that files are identical.
>
> Best,
> Stamatis
>
> [1] https://lists.apache.org/thread/j0fh9hhxdc9lc7n228kwj6xcs0z9hpb3
>
> On Thu, Oct 20, 2022 at 8:36 PM Julian Hyde 
> wrote:
>
> > Do you think that RelOptRulesTest should fail if the resources are not
> > sorted alphabetically? (I thought it did that at some point… I may be
> > wrong. Anyway, I am a fan of ‘fail fast’. The person who introduces the
> > error should be the person to fix it.)
> >
> > > On Oct 20, 2022, at 9:20 AM, Alessandro Solimando <
> > alessandro.solima...@gmail.com> wrote:
> > >
> > > Hello everyone,
> > > I have recently added tests to RelOptRulesTest.java, took the chance to
> > > update (maven instruction with gradle instructions, path has changed)
> and
> > > improved them a little bit.
> > >
> > > While testing the commands I have noticed that few PRs in the last
> months
> > > had tests added to RelOptRulesTest.java with a manual update of
> > > RelOptRulesTest.xml (possibly due to the stale instructions).
> > >
> > > Manual updates often suffer from formatting issues, stale/wrong SQL
> > > statements, different test order.
> > >
> > > Note that those discrepancies are not caught by running tests/CI, but
> > it's
> > > nonetheless problematic, especially because it forbids folks down the
> > line
> > > to re-generate the file automatically, because their PRs would carry
> the
> > > changes from past work.
> > >
> > > In https://github.com/apache/calcite/pull/2944 I will put back on
> track
> > > RelOptRulesTest.xml, please try to follow the procedure in the javadoc
> of
> > > RelOptRulesTest.java and let me know if we can improve them even
> further
> > in
> > > your opinion.
> > >
> > > Best regards,
> > > Alessandro
> >
> >
>

Re: RelOptRulesTest.java and RelOptRulesTest.xml

[Stamatis Zampetakis]

Thanks for taking care of this Alessandro!

I believe the resources are still sorted as per [1] but it appears that
there are still a few manual edits to the file.

If we want to prevent such changes in the future it may be a good idea to
add a post-class check In RelOptRulesTest (or somewhere in DiffRepo)
checking that files are identical.

Best,
Stamatis

[1] https://lists.apache.org/thread/j0fh9hhxdc9lc7n228kwj6xcs0z9hpb3

On Thu, Oct 20, 2022 at 8:36 PM Julian Hyde  wrote:

> Do you think that RelOptRulesTest should fail if the resources are not
> sorted alphabetically? (I thought it did that at some point… I may be
> wrong. Anyway, I am a fan of ‘fail fast’. The person who introduces the
> error should be the person to fix it.)
>
> > On Oct 20, 2022, at 9:20 AM, Alessandro Solimando <
> alessandro.solima...@gmail.com> wrote:
> >
> > Hello everyone,
> > I have recently added tests to RelOptRulesTest.java, took the chance to
> > update (maven instruction with gradle instructions, path has changed) and
> > improved them a little bit.
> >
> > While testing the commands I have noticed that few PRs in the last months
> > had tests added to RelOptRulesTest.java with a manual update of
> > RelOptRulesTest.xml (possibly due to the stale instructions).
> >
> > Manual updates often suffer from formatting issues, stale/wrong SQL
> > statements, different test order.
> >
> > Note that those discrepancies are not caught by running tests/CI, but
> it's
> > nonetheless problematic, especially because it forbids folks down the
> line
> > to re-generate the file automatically, because their PRs would carry the
> > changes from past work.
> >
> > In https://github.com/apache/calcite/pull/2944 I will put back on track
> > RelOptRulesTest.xml, please try to follow the procedure in the javadoc of
> > RelOptRulesTest.java and let me know if we can improve them even further
> in
> > your opinion.
> >
> > Best regards,
> > Alessandro
>
>

Re: RelOptRulesTest.java and RelOptRulesTest.xml

[Julian Hyde]

Do you think that RelOptRulesTest should fail if the resources are not sorted 
alphabetically? (I thought it did that at some point… I may be wrong. Anyway, I 
am a fan of ‘fail fast’. The person who introduces the error should be the 
person to fix it.)

> On Oct 20, 2022, at 9:20 AM, Alessandro Solimando 
>  wrote:
> 
> Hello everyone,
> I have recently added tests to RelOptRulesTest.java, took the chance to
> update (maven instruction with gradle instructions, path has changed) and
> improved them a little bit.
> 
> While testing the commands I have noticed that few PRs in the last months
> had tests added to RelOptRulesTest.java with a manual update of
> RelOptRulesTest.xml (possibly due to the stale instructions).
> 
> Manual updates often suffer from formatting issues, stale/wrong SQL
> statements, different test order.
> 
> Note that those discrepancies are not caught by running tests/CI, but it's
> nonetheless problematic, especially because it forbids folks down the line
> to re-generate the file automatically, because their PRs would carry the
> changes from past work.
> 
> In https://github.com/apache/calcite/pull/2944 I will put back on track
> RelOptRulesTest.xml, please try to follow the procedure in the javadoc of
> RelOptRulesTest.java and let me know if we can improve them even further in
> your opinion.
> 
> Best regards,
> Alessandro

RelOptRulesTest.java and RelOptRulesTest.xml

[Alessandro Solimando]

Hello everyone,
I have recently added tests to RelOptRulesTest.java, took the chance to
update (maven instruction with gradle instructions, path has changed) and
improved them a little bit.

While testing the commands I have noticed that few PRs in the last months
had tests added to RelOptRulesTest.java with a manual update of
RelOptRulesTest.xml (possibly due to the stale instructions).

Manual updates often suffer from formatting issues, stale/wrong SQL
statements, different test order.

Note that those discrepancies are not caught by running tests/CI, but it's
nonetheless problematic, especially because it forbids folks down the line
to re-generate the file automatically, because their PRs would carry the
changes from past work.

In https://github.com/apache/calcite/pull/2944 I will put back on track
RelOptRulesTest.xml, please try to follow the procedure in the javadoc of
RelOptRulesTest.java and let me know if we can improve them even further in
your opinion.

Best regards,
Alessandro