Skip to content

Conversation

@beliefer
Copy link
Contributor

@beliefer beliefer commented Jul 29, 2022

What changes were proposed in this pull request?

This PR expect to improve the comments about null tracking for UnsafeRow.
The old comment of UnsafeRow have confused text [null bit set].
In fact, the portion is a lot of bit or bit array which does't always be null.
On the other hand, it tell users nothing. we need the information more clear.

Why are the changes needed?

Improve the comments about null tracking for UnsafeRow.

Does this PR introduce any user-facing change?

'No'.
Just update comments.

How was this patch tested?

N/A

@github-actions github-actions bot added the SQL label Jul 29, 2022
@beliefer
Copy link
Contributor Author

@cloud-fan
Copy link
Contributor

what's wrong with "null bit set"? A bit set for null tracking.

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don't particularly find this confusing too

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not sure if this is an improvement or not, but I feel new sentence looks like becoming repeating itself a little.

The null-tracking portion is used for null tracking 

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's not a big deal, let's merge this for @beliefer and move on to more important PRs. :)

@beliefer beliefer force-pushed the UnsafeRow_comment branch from a7d94db to d91c561 Compare August 4, 2022 07:03
@beliefer beliefer force-pushed the UnsafeRow_comment branch from e0500c2 to 65f30b2 Compare August 6, 2022 02:44
@cloud-fan
Copy link
Contributor

thank, merging to master!

@cloud-fan cloud-fan closed this in bf42b89 Aug 8, 2022
@beliefer
Copy link
Contributor Author

beliefer commented Aug 8, 2022

@cloud-fan @dongjoon-hyun @HyukjinKwon Thank you !

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants