rubenada edited a comment on pull request #2440:
URL: https://github.com/apache/calcite/pull/2440#issuecomment-868976929
> Conventionally when defining a lambda for relFn we call the RelBuilder b,
not relBuilder. It makes the code more concise when we use the RelBuilder
multiple times to create expressions. Can you stick to that convention.
Done.
> I think the relFn parameter and field need a @Nullable annotation.
Annotation added.
> Why are there so many renames of "planBefore" to "planAfter" and vice
versa? I can't tell whether these are real changes or just instability in the
file format. If you can, preserve the file format. Generally "planBefore"
should occur before "planAfter" (even though that is not alphabetical).
This is because I have homogenized several tests. Originally, some of the
tests were checking "manually" a certain plan with some boilerplate code e.g.
```
RelNode output = hepPlanner.findBestExp();
final String planAfter = NL + RelOptUtil.toString(output);
final DiffRepository diffRepos = getDiffRepos();
diffRepos.assertEquals("planAfter", "${planAfter}", planAfter);
SqlToRelTestBase.assertValid(output);
```
This has been refactored to reuse `RelOptTestBase#Sql` methods:
```
relFn(relFn).withRule(...).check();
...
relFn(relFn).withRule(...).checkUnchanged();
```
- `check`: for the plans that must change when the rules are applied. They
require `${planBefore}` and `${planAfter}`, and they must be different.
- `checkUnchanged`: for the plans that must not change when rules are
applied. They require only `${planBefore}` (which is used to times, before and
after the rules application.
Originally, some tests were checking an unchanged plan by checking manually
the after plan, and they only defined `${planAfter}` in the xml file. With the
proposed homogenization, these tests are now using `checkUnchanged` (and the
plan is defined as `${planBefore}`).
Analogously, some tests were checking a changed plan by manually checking
the after plan, and they only defined `${planAfter}` in the xml file. These
tests are now using `check`, and they define also their corresponding
`${planBefore}` too.
To sum up, we can have tests (the unchanged ones) that define only
`${planBefore}`; and tests (the changed ones) that defines `${planBefore}` and
`${planAfter}` (which must be different). I agree that the appropriate order
for the latter is `${planBefore}` and then `${planAfter}`, I have tried to
apply this convention; @julianhyde do you have any concrete example where this
order is inversed in the patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
