Skip to content

[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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dengziming
Copy link
Member

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:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (don't know, maybe)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

@flinkbot
Copy link
Collaborator

flinkbot commented May 24, 2025

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@dengziming
Copy link
Member Author

@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) {
Copy link
Contributor

@davidradl davidradl May 27, 2025

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?

Copy link
Member Author

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.

Copy link
Contributor

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);
Copy link
Contributor

@davidradl davidradl May 27, 2025

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);

Copy link
Member Author

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).

Copy link
Member Author

@dengziming dengziming left a 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) {
Copy link
Member Author

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);
Copy link
Member Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants