julianhyde edited a comment on pull request #2529: URL: https://github.com/apache/calcite/pull/2529#issuecomment-923488887
I second @asolimando's point about merging. When a PR is under review, don't rebase (unless asked to), don't squash commits, don't amend commits, just add new commits. It makes things easier for reviewers. We know how to do `git diff HEAD~5` if we need to. I think it would be useful if you describe in brief a javadoc sentence the particular feature that each each test is testing. It's difficult to capture this in the test name - for example, the test case "testGroupSetMultipleLiteralsOrder" contains neither `GROUPING SET` nor `ORDER BY`. I understand from reading the test that you are interested in a query that contains literals and the literals are in a different order in the SELECT clause to the GROUP BY clause. I'm not sure that this bug requires 70 lines of new tests, because the fix simply removes literals. Since all tests use the same MV, I would compress them all into one method. The one case you have not tested is a query that groups by a literal and no columns, e.g. `GROUP BY 'a'`. The semantics are non-obvious when the table is empty. It warrants its own test. -- 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]
