[FLINK-37840] [table] Row writer should honor null uncompact BigDecimal and Timestamp #26594
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.
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.
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.
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.
why is this setNullBit(pos); but others are setNullAt(pos);
There was a problem hiding this comment.
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.
@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.
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.
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