[CALCITE-6561] Add GROUP BY test cases for AS MEASURE aggregates #3946
Conversation
|
| @@ -26,6 +26,136 @@ from emp; | |||
|
|
|||
| !update | |||
|
|
|||
| create view empm_comm as | |||
There was a problem hiding this comment.
I wouldn't create a view for just one query. (Unless a view is necessary to demonstrate the behavior, and a CTE wouldn't do it.)
If it's not too disruptive of later tests, you could add avg_comm to empm.
| !ok | ||
|
|
||
| # single GROUPING SETS group with AS MEASURE reference | ||
| SELECT job, year(hiredate) as hire_year, avg_sal FROM empm |
There was a problem hiding this comment.
if you're going to take 4 lines for GROUP BY, at least put FROM on its own line. (I find queries much more readable if clauses are on separate lines.)
There was a problem hiding this comment.
This file is ordered by complexity - simple queries first - and groups related concepts. GROUPING SETS is relatively advanced functionality, so I would move these queries further down the measure.iq file.
|
This PR feels too small. In calcite, a commit that is merged to main is generally at the grain of a jira case, and a jira case is a bug or a feature. Commits merged to main are rarely small tasks. (For small tasks such as fixing typos, we recommend PRs without a jira case.) I see that CALCITE-6561, the identifier of this PR, is logged as a bug, so that's a good start. I'd start a dev branch with 6561 in its name, use the bug description as the PR summary, and add 'Add test case for [CALCITE-6561] Measure query gives incorrect result when GROUP BY key is NULL' as the first commit. You can note in the commit message that there are commented-out test cases for other bugs. When the bug is fixed, all the commits on the branch will be squash-merged to main. |
Add GROUP BY test cases for AS MEASURE aggregates
also add failing test cases behind
!if()conditions: