Skip to content

Avoid array list shift in ChunkedSliceOutput#17031

Merged
findepi merged 2 commits intotrinodb:masterfrom
findepi:findepi/avoid-array-list-shift-in-chunkedsliceoutput-29e2c3
Apr 17, 2023
Merged

Avoid array list shift in ChunkedSliceOutput#17031
findepi merged 2 commits intotrinodb:masterfrom
findepi:findepi/avoid-array-list-shift-in-chunkedsliceoutput-29e2c3

Conversation

@findepi
Copy link
Copy Markdown
Member

@findepi findepi commented Apr 14, 2023

Previously, the buffer allocation would remove first element of an ArrayList, causing a shift of all elements. This is rather ignorable: buffers get allocated bigger and bigger, so there aren't many buffer allocations. Still, it can be avoided, by keeping buffers on one list instead of two.

@findepi findepi added the no-release-notes This pull request does not require release notes entry label Apr 14, 2023
@cla-bot cla-bot bot added the cla-signed label Apr 14, 2023
@findepi findepi force-pushed the findepi/avoid-array-list-shift-in-chunkedsliceoutput-29e2c3 branch from 4a554c1 to 69288a4 Compare April 14, 2023 10:24
Copy link
Copy Markdown
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

LGTM. Is it covered by some tests?

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Apr 14, 2023

LGTM. Is it covered by some tests?

I am convinced this is not dead code, but would defer to @dain for this.

Unless you mean explicit unit test for this class, then I don't think so

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Apr 14, 2023

@losipiuk there indeed is coverage and i made some mistake

Error:  Failures: 
Error:    TestOrcOutputBuffer.testWriteHugeByteChucks:36 » IndexOutOfBounds Index 1 out ...
Error:    TestOrcReader>AbstractTestOrcReader.testBinaryDirectSequence:485 » IndexOutOfBounds
Error:    TestOrcReader>AbstractTestOrcReader.testCharDirectSequence:451 » IndexOutOfBounds
Error:    TestOrcReader>AbstractTestOrcReader.testDecimalSequence:243 » IndexOutOfBounds
Error:    TestOrcReader>AbstractTestOrcReader.testDoubleSequence:235 » IndexOutOfBounds ...
Error:    TestOrcReader.testDoubleSequenceFull:34 » IndexOutOfBounds Index 1 out of boun...
Error:    TestOrcReader>AbstractTestOrcReader.testFloatSequence:214 » IndexOutOfBounds I...
Error:    TestOrcReader>AbstractTestOrcReader.testLongDirect2:146->AbstractTestOrcReader.testRoundTripNumeric:176 » IndexOutOfBounds
Error:    TestOrcReader>AbstractTestOrcReader.testLongSequence:110->AbstractTestOrcReader.testRoundTripNumeric:182 » IndexOutOfBounds
Error:    TestOrcReader>AbstractTestOrcReader.testLongSequenceWithHoles:127->AbstractTestOrcReader.testRoundTripNumeric:182 » IndexOutOfBounds
Error:    TestOrcReader>AbstractTestOrcReader.testNegativeLongSequence:120->AbstractTestOrcReader.testRoundTripNumeric:182 » IndexOutOfBounds
Error:    TestOrcReader>AbstractTestOrcReader.testStringDirectSequence:415 » IndexOutOfBounds
Error:    TestOrcReader>AbstractTestOrcReader.testUuidDictionarySequence:528 » IndexOutOfBounds
Error:    TestOrcReader>AbstractTestOrcReader.testUuidDirectSequence:517 » IndexOutOfBounds
Error:    TestOrcWriter.testWriteOutputStreamsInOrder:104 » IndexOutOfBounds Index 1 out...
Error:    TestSliceDictionaryColumnReader.testDictionaryReaderUpdatesRetainedSize:57 » IndexOutOfBounds
Error:    TestSliceDictionaryColumnWriter.testDirectConversion:78 » IndexOutOfBounds Ind...
Error:    TestLongDecimalStream.test:51->AbstractTestValueStream.testWriteValue:50 » IndexOutOfBounds
Error:    TestShortDecimalStream.test:50->AbstractTestValueStream.testWriteValue:50 » IndexOutOfBounds

will fix shortly

Previously, the buffer allocation would remove first element of an
`ArrayList`, causing a shift of all elements. This is rather ignorable:
buffers get allocated bigger and bigger, so there aren't many buffer
allocations. Still, it can be avoided, by keeping buffers on one list
instead of two.
@findepi findepi force-pushed the findepi/avoid-array-list-shift-in-chunkedsliceoutput-29e2c3 branch from 69288a4 to 9661bd9 Compare April 14, 2023 12:37
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Apr 17, 2023

CI #17052

@findepi findepi merged commit 87828e1 into trinodb:master Apr 17, 2023
@findepi findepi deleted the findepi/avoid-array-list-shift-in-chunkedsliceoutput-29e2c3 branch April 17, 2023 06:52
@github-actions github-actions bot added this to the 414 milestone Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

2 participants