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-14053] [blink-planner] DenseRankAggFunction.accumulateExpressions. it should be thinki… #9966
Conversation
…ng about a corner case when the order by expression equals to inital lastValue.
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit c877c93 (Thu Oct 24 18:07:50 UTC 2019) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
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 fixing this @liuyongvs
import static org.apache.flink.table.expressions.ExpressionBuilder.ifThenElse; | ||
import static org.apache.flink.table.expressions.ExpressionBuilder.literal; | ||
import static org.apache.flink.table.expressions.ExpressionBuilder.plus; | ||
import static org.apache.flink.table.expressions.ExpressionBuilder.*; |
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.
Please don't use import *
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.
got it
// sequence = if (lastValues equalTo orderKeys) sequence else sequence + 1 | ||
accExpressions[0] = ifThenElse(orderKeyEqualsExpression(), sequence, plus(sequence, literal(1L))); | ||
// sequence = if (lastValues equalTo orderKeys and sequence != 0) sequence else sequence + 1 | ||
accExpressions[0] = ifThenElse(and(orderKeyEqualsExpression(), not(equalTo(sequence, literal(0L)))), sequence, plus(sequence, literal(1L))); |
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.
inside orderKeyEqualsExpression()
, we already compare the last_value with null which is the initial value. Why this conflicts with the order by key's value? Do you mean the order by key is null?
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.
you scan see the unit test i commit. it will be zero rank
Your base branch seems to be wrong, please use latest master branch and create a fix based on that. |
Thanks, i found the conflict when i commited because of my personal respository, which it not the latest. |
Hi @KurtYoung , I have already recolved the conflicts. |
@liuyongvs There are some compilation errors, please fix it first. |
|
||
// deal with input with 0 as the first row's rank field | ||
checkResult( | ||
"SELECT f, dense_rank() over (order by d) FROM Table5", |
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.
If I don't misunderstood anything, your test seems to be wrong.
The data of column d is 1, 2, 2, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5, 5, 5
, how could it possible that the dense_rank
on column d becomes 1, 2, 3, ..., 14, 15
?
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.
sorry, i made a mistake becase of careless
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.
it should be SELECT f, dense_rank() over (order by f) FROM Table5
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.
LGTM, thanks for the fix.
Merging this, but please don't open a pull request against master branch based on a non-master branch next time.. @liuyongvs |
What is the purpose of the change
Brief change log
Does this pull request potentially affect one of the following parts:
Documentation
Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)