Skip to content

fix array type strategy write size tracking#12150

Merged
suneet-s merged 2 commits intoapache:masterfrom
clintropolis:fix-array-type-strategy-size-tracking
Jan 13, 2022
Merged

fix array type strategy write size tracking#12150
suneet-s merged 2 commits intoapache:masterfrom
clintropolis:fix-array-type-strategy-size-tracking

Conversation

@clintropolis
Copy link
Copy Markdown
Member

Description

This PR fixes a bug introduced in #11888 for the ARRAY type strategy write method, where the size tracking of remaining buffer space had a silly math bug that made size tracking go wildly wrong the longer the array was. I was incorrectly subtracting the total so far bytes written for the whole array, rather than only the bytes for each element written, so it "used" up the space far far quicker than the actual number of bytes written 😛

I added a test, which fails prior to the changes in this PR, that writes an array that is nearly the size of the test buffer to ensure that size tracking allows the full use of the buffer.

I also fixed up a couple of spots that mutate buffer position in NullableTypeStrategy that should've been wrapped in a try..finally, so now they are.

This PR has:

  • been self-reviewed.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.

Copy link
Copy Markdown
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

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

LGTM

@suneet-s suneet-s merged commit 1dba089 into apache:master Jan 13, 2022
@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022
sachinsagare pushed a commit to sachinsagare/druid that referenced this pull request Nov 3, 2022
* fix array type strategy write size tracking

* fix checkstyle

(cherry picked from commit 1dba089)
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.

4 participants