xuyangzhong commented on code in PR #19797:
URL: https://github.com/apache/flink/pull/19797#discussion_r1599910377
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/plan/batch/sql/agg/OverAggregateTest.scala:
##########
@@ -47,6 +48,18 @@ class OverAggregateTest extends TableTestBase {
util.verifyExecPlan("SELECT c, SUM(a) OVER (ORDER BY b) FROM MyTable")
}
+ @Test
+ def testDenseRankOnOrder(): Unit = {
Review Comment:
The plan tests in batch mode appear not to be able to capture the
modifications introduced by this PR, and I apologize for mentioning the unit
test for the plan earlier.
I've attempted this case, and it seems only possible to test it within an
integration test in stream mode. I propose we add another testRankOnOver test
specifically for testing the RANK() function.
Additionally, there seems to be a potential risk of encountering a NPE in
`AggFunctionFactory#createPercentRankAggFunction` (even though `PercentRank` is
not currently supported in streaming mode, the code suggests such a risk).
Maybe we'd better address this as well?
// ----------
// Broadening the discussion a bit, perhaps we should consider fundamentally
preventing the `orderKeyIndexes`, which isn't marked as Nullable, from being
null, although this would require substantial changes 🤔. I agree that we can
just avoid huge changes as demonstrated by this PR.
--
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]
