-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[FLINK-37840] [table] Row writer should honor null uncompact BigDecimal and Timestamp #26594
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
base: master
Are you sure you want to change the base?
Conversation
@wuchong can you help look if this change is reasonable? |
@@ -372,7 +372,11 @@ public void setDecimal(int pos, DecimalData value, int precision) { | |||
|
|||
if (DecimalData.isCompact(precision)) { | |||
// compact format | |||
setLong(pos, value.toUnscaledLong()); | |||
if (value == 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.
this if code is duplicated more than once by this fix, can we refactor to a method remove the duplication.
Better still could we have an enumeration for each type then use the enumeration to call a common method. Maybe the enumeration could be something likesomething like:
SetLongSemantics( Object type, boolean isUnScaledLong) {
this.type = type;
this.isUnScaledLong = isUnScaledLong;
}
TimestampDataValueAsLong(TimestampData, true),
TimestampDataValueAsMilliseconds(TimestampData, false),
....
If setNullBit is import then include that.
WDYT?
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 this suggestion, the motivation makes sense. The effort required to unify these abstractions is significant, and it's overkill to do this in the PR, so I just left a todo here for now.
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.
My concern is that we have added a lot of duplicate code with the if elses
added in this fix. It seems prudent to have a common method so the logic does not diverge in the different cases.
@snuyanzin WDYT?
assert value != null; | ||
writeLong(pos, value.toUnscaledLong()); | ||
if (value == null) { | ||
setNullBit(pos); |
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 is this setNullBit(pos);
but others are setNullAt(pos);
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.
Good catch, I reviewed the code, setNullBit
only sets the null-tracking bit of this field, and setNullAt
sets null-bit and sets the fixed-length data to 0, so it should be setNullAt(pos)
.
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.
@davidradl your comments are useful, take a look again please.
@@ -372,7 +372,11 @@ public void setDecimal(int pos, DecimalData value, int precision) { | |||
|
|||
if (DecimalData.isCompact(precision)) { | |||
// compact format | |||
setLong(pos, value.toUnscaledLong()); | |||
if (value == 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.
Thanks for this suggestion, the motivation makes sense. The effort required to unify these abstractions is significant, and it's overkill to do this in the PR, so I just left a todo here for now.
assert value != null; | ||
writeLong(pos, value.toUnscaledLong()); | ||
if (value == null) { | ||
setNullBit(pos); |
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.
Good catch, I reviewed the code, setNullBit
only sets the null-tracking bit of this field, and setNullAt
sets null-bit and sets the fixed-length data to 0, so it should be setNullAt(pos)
.
What is the purpose of the change
Fix NPE when writing null uncompact BigDecimal and Timestamp
Brief change log
since we support passing null compact BigDecimal and Timestamp, we should also support passing null uncompact ones.
Verifying this change
Please make sure both new and modified tests in this PR follow the conventions for tests defined in our code quality guide.
This change added tests and can be verified as follows:
Adding more test cases to existing cases.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (don't know, maybe)Documentation