ORC-1130:[C++] Suppress the present stream when the data stream has no null value #1067
Conversation
| dataBuffer->resize(0); | ||
| return dataSize; | ||
| } | ||
| void BufferedOutputStream::suppress() { |
There was a problem hiding this comment.
Could you add an empty line before this?
fix the code format
c++/src/ColumnWriter.cc
Outdated
| notNullEncoder->add(batch.notNull.data() + offset, numValues, incomingMask); | ||
| const char* notNull = batch.notNull.data() + offset; | ||
| notNullEncoder->add(notNull, numValues, incomingMask); | ||
| for (uint64_t i = 0; i < numValues; ++i) { |
There was a problem hiding this comment.
I think this can be skipped if hasNullValue is already true.
c++/test/TestReader.cc
Outdated
| EXPECT_EQ(1, nestedUnionBatch.offsets.data()[1]); | ||
| } | ||
|
|
||
| TEST(TestReadIntent, testSuppressPresentStream) { |
There was a problem hiding this comment.
This is not part of the ReadIntent tests. Maybe renaming TestReadIntent to TestReader is better.
There was a problem hiding this comment.
I think this feature is mainly a modification on the writer side. So I move the test into TestWriter.cc
c++/test/TestReader.cc
Outdated
| reader->createRowReader(RowReaderOptions()); | ||
| auto batch = rowReader->createRowBatch(2048); | ||
| EXPECT_TRUE(rowReader->next(*batch)); | ||
| EXPECT_EQ(rowCount, batch->numElements); |
There was a problem hiding this comment.
Could you also verify the content of the read batch?
There was a problem hiding this comment.
Sure, based on your suggestion, I added more coverage to this test.
c++/test/TestReader.cc
Outdated
| EXPECT_EQ(stream.column(), 1UL); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you add the test coverage for seek()? It can make sure the rowIndex posistions are still correct.
| notNullEncoder->add(notNull, numValues, incomingMask); | ||
| for (uint64_t i = 0; i < numValues; ++i) { | ||
| if (!notNull[i]) { | ||
| hasNullValue = true; |
There was a problem hiding this comment.
I'd suggest checking hasNullValue in line 145 or 146 because hasNullValue may already be set by previous nulls.
There was a problem hiding this comment.
In addition, we should check batch.hasNulls as a shortcut.
There was a problem hiding this comment.
This logic has been changed to take the or operation with batch.hasNulls.
| // written data can be just ignored because they are only flushed in memory | ||
| outputStream->suppress(); | ||
|
|
||
| numLiterals = 0; |
There was a problem hiding this comment.
Why not nullify buffer? I'd add a reset() function to clear the state and use it here and in the ByteRleEncoderImpl constructor.
There was a problem hiding this comment.
I don't find the reset function, so I add this function by the way.
… the test coverage for seek
|
I have merged it. Thanks @coderex2522 @dongjoon-hyun @stiga-huang ! |
|
Thank you, @coderex2522 , @wgtmac and @stiga-huang . |
…o null value This closes apache#1067
Why are the changes needed?
if the data stream has no null value, the present stream of this column is redundant.
How was this patch tested?
Through the UT TestReadIntent.testSuppressPresentStream.