-
Notifications
You must be signed in to change notification settings - Fork 807
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] Update using partial columns. #3299
base: master
Are you sure you want to change the base?
Conversation
@@ -441,20 +439,6 @@ default Long generateNullable(InternalRow row, int i) { | |||
|
|||
private static class SequenceGeneratorVisitor extends DataTypeDefaultVisitor<Generator> { | |||
|
|||
@Override |
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.
Why remove here?
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.
The doc doesn't mention support for STRING, so I removed it. Otherwise, using STRING might lead to parsing errors.
For fields..sequence-group, valid comparative data types include: DECIMAL, TINYINT, SMALLINT, INTEGER, BIGINT, FLOAT, DOUBLE, DATE, TIME, TIMESTAMP, and TIMESTAMP_LTZ.
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.
What we need to do is supporting string using UserDefinedSeqComparator
way.
If you are interested in this, you can do it in a separate PR, please let this PR clean.
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.
ok, the code has been restored. will submit a new pr.
Hi @hzjhjjyy , is this come from user requirement? I understand this optimization, but I'm hesitant to move forward with it. |
I think this is to improve the efficiency and scope of update. Naturally, I’m also willing to decide how to handle this pr based on your advice. |
Hi @hzjhjjyy for your inputs:
Considering these two points, and the changes made by this PR to the current topology are not very worthwhile. |
Hi @JingsongLi .
Overall, this pr aims to support the feature of using partial columns in updates in flink. I wonder if my explanation clarifies and captures your intent? |
Yes, I got your point, but my point is just "Is our modification worth it", we can wait for these requirements to emerge. |
Purpose
FLINK-32001 has been resolved, so partial columns can now be used for updates.
Different merge engines have different requirements(aggregation has also been supported for updates):
Tests
BatchUpdateWithPartialColumnsITCase (all aggregations, sequence field/group and changelog have been tested)
API and Format
Documentation