affo commented on code in PR #24270:
URL: https://github.com/apache/flink/pull/24270#discussion_r1492347207
##########
flink-table/flink-table-planner/src/test/resources/org/apache/flink/table/planner/plan/batch/sql/agg/AggregateReduceGroupingTest.xml:
##########
@@ -765,13 +765,13 @@ HashAggregate(isMerge=[false], groupBy=[a1],
auxGrouping=[d1], select=[a1, d1, C
</TestCase>
<TestCase name="testSingleAggOnlyConstantGroupKey">
<Resource name="sql">
- <![CDATA[SELECT count(c1) FROM T1 GROUP BY 1, true]]>
Review Comment:
At first I was wondering: Why changing the integer to string?
Does not seem to be necessary in this PR, as this test is testing grouping
by a constant.
But now I understand that those `1`s now take a different meaning and I like
the approach to simply change it to a string 👍
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/stream/sql/AggregateITCase.scala:
##########
@@ -346,6 +346,73 @@ class AggregateITCase(aggMode: AggMode, miniBatch:
MiniBatchMode, backend: State
assertThat(sink.getRetractResults.sorted).isEqualTo(expected.sorted)
}
+ @TestTemplate
+ def testGroupRowsByColumnOrdinals1(): Unit = {
+ // this case covers LongArrayValueWithRetractionGenerator and
LongValueWithRetractionGenerator
+ val data = new mutable.MutableList[(Int, Long, String)]
+ data.+=((1, 1L, "A"))
+ data.+=((1, 1L, "A"))
+ data.+=((1, 1L, "A"))
+ data.+=((2, 2L, "B"))
+ data.+=((3, 2L, "B"))
+ data.+=((4, 3L, "C"))
+ data.+=((5, 3L, "C"))
+ data.+=((6, 3L, "C"))
+ data.+=((7, 4L, "B"))
+ data.+=((8, 4L, "A"))
+ data.+=((9, 4L, "D"))
+ data.+=((10, 4L, "E"))
+ data.+=((11, 5L, "A"))
+ data.+=((12, 5L, "B"))
+
+ val t = failingDataSource(data).toTable(tEnv, 'a, 'b, 'c)
+ tEnv.createTemporaryView("T", t)
+
+ val sql =
+ """
+ | SELECT b, sum(a)
+ | FROM T
+ | GROUP BY 1
+ """.stripMargin
+
+ val t1 = tEnv.sqlQuery(sql)
+ val sink = new TestingRetractSink
+ t1.toRetractStream[Row].addSink(sink).setParallelism(1)
+ env.execute()
+
+ val expected = List("1,3", "2,5", "3,15", "4,34", "5,23")
+ assertThat(sink.getRetractResults.sorted).isEqualTo(expected.sorted)
+ }
+
+ @TestTemplate
+ def testGroupRowsByColumnOrdinals2(): Unit = {
Review Comment:
Can we have more meaningful names for tests?
For example `testGroupRowsByMultipleColumnOrdinals`
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/stream/sql/AggregateITCase.scala:
##########
@@ -346,6 +346,73 @@ class AggregateITCase(aggMode: AggMode, miniBatch:
MiniBatchMode, backend: State
assertThat(sink.getRetractResults.sorted).isEqualTo(expected.sorted)
}
+ @TestTemplate
+ def testGroupRowsByColumnOrdinals1(): Unit = {
Review Comment:
Can we have more meaningful names for tests?
For example `testGroupRowsBySingleColumnOrdinals`
--
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]
