New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-27741][table-planner] Fix NPE when use dense_rank() and rank()… #19797
base: master
Are you sure you want to change the base?
Conversation
… in over aggregation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution on fixing this bug. The code looks good to me overall and I left some comments on it. ; )
@@ -98,7 +98,7 @@ protected Expression orderKeyEqualsExpression() { | |||
equalTo(lasValue, operand(i))); | |||
} | |||
Optional<Expression> ret = Arrays.stream(orderKeyEquals).reduce(ExpressionBuilder::and); | |||
return ret.orElseGet(() -> literal(true)); | |||
return ret.orElseGet(() -> literal(false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I just wander why this value should be changed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xuyangzhong The default value of true will cause the sort value to be unchanged, which I don't think conforms to the semantics of sorting.
@@ -159,6 +159,66 @@ class OverAggregateITCase(mode: StateBackendMode) extends StreamingWithStateTest | |||
assertEquals(expected.sorted, sink.getAppendResults.sorted) | |||
} | |||
|
|||
@Test | |||
def testDenseRankOnOver(): Unit = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can add some test cases for testing plan, not only just for ITCases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @xuyangzhong . Thanks for your review and sorry for delay response. I didn't find the UT of AggFunctionFactory, so I borrowed the test for ROW_NUMBER. And I haven't waited for the community's reply, I can add corresponding unit tests later if necessary.
Fix NPE when use dense_rank() and rank() in over aggregation.
What is the purpose of the change
This pull request fixes NullPointException when use DENSE_RANK() and RANK() for an append stream.
Brief change log
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation