my7ym commented on a change in pull request #1186: [CALCITE-3003]
AssertionError when GROUP BY nested field
URL: https://github.com/apache/calcite/pull/1186#discussion_r279442992
##########
File path:
core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -2679,6 +2679,66 @@ public void testSelectNestedColumnType() {
sql(sql).ok();
}
+ /**
+ * Test case for <a
href="https://issues.apache.org/jira/browse/CALCITE-3003";>[CALCITE-3003]
+ * AssertionError when GROUP BY nested field</a>.
+ */
+ @Test
+ public void testGroupByNestedColumn() {
+ final String sql =
+ "select\n"
+ + " coord.x,\n"
+ + " coord_ne.sub.a,\n"
+ + " avg(coord.y)\n"
+ + "from\n"
+ + " customer.contact_peek\n"
+ + "group by\n"
+ + " coord_ne.sub.a,\n"
+ + " coord.x";
+ sql(sql).ok();
+ }
+
+ /**
+ * Test case for <a
href="https://issues.apache.org/jira/browse/CALCITE-3003";>[CALCITE-3003]
+ * AssertionError when GROUP BY nested field</a>.
+ */
+ @Test
+ public void testGroupingSetsWithNestedColumn() {
+ final String sql =
+ "select\n"
+ + " coord.x,\n"
+ + " coord.\"unit\",\n"
+ + " coord_ne.sub.a,\n"
+ + " avg(coord.y)\n"
+ + "from\n"
+ + " customer.contact_peek\n"
+ + "group by\n"
+ + " grouping sets (\n"
+ + " (coord_ne.sub.a, coord.x, coord.\"unit\"),\n"
+ + " (coord.x, coord.\"unit\")\n"
+ + " )";
+ sql(sql).ok();
+ }
+
+ /**
+ * Test case for <a
href="https://issues.apache.org/jira/browse/CALCITE-3003";>[CALCITE-3003]
+ * AssertionError when GROUP BY nested field</a>.
+ */
+ @Test
Review comment:
For the comment style:
There seems to be two conventions in the codebase. One is commenting with
JIRA link and the other one is using your approach.
If you don't have strong opinions on this, I prefer JIRA link because:
1. The `similar to` approach is like linking several tests to one unit test.
When you see individual tests, you don't have full context on them. And what if
the linked_to test are updated or even deleted? And if you really want to link
all tests together, git blame or simple text search give all the information.
2. The JIRA link is a little verbose, but (maybe to me only?) it is
acceptable.
3. If tests are logically complex and hard to extract common components
among them, I agree that mark an extra `similar to` could help understanding.
But the tests here are super and intuitive.
If you have strong opinions let me know, I will change it.
For the description:
Do you mind if I change my test name to
testInvalidGroupBy_withInvalidTableName, instead of adding a comment?
Let me know your final take. I will update the diff.
Thanks for review
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
