[GitHub] [calcite] rubenada edited a comment on pull request #2440: RelOptRulesTest improvements

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org