ORC-1087: [C++] Handle unloaded seek positions when seeking in an uncompressed chunk #1008

stiga-huang · 2022-01-11T03:37:28Z

What changes were proposed in this pull request?

This PR fixes an unhandled case when seeking in an uncompressed chunk.

Why are the changes needed?

The bug causes position overflow and fails the reader when encountered the unhandled case. Some background:

Compressed streams are compressed in chunks. If the compressed size of a chunk is larger than the original size, the original (uncompressed) chunk will be kept. The chunk header records the chunk length and whether it's compressed.
Seek position in a compressed stream is encoded into 2 numbers: pos in the input stream and pos in the chunk. The first number locates the chunk header. The second number locates the position in the decompressed chunk.
Compressed chunks are decompressed in a whole so the whole chunk is in the output buffer. Uncompressed chunks don't need decompression so the input buffer is used directly. However, the chunk could be read in pieces depending on the block size of the input stream. So the seek position might not be loaded yet.

The unhandled case is: the seek position is in the current chunk but posInChunk is not loaded yet. We should skip the remaining bytes to seek to it.

How was this patch tested?

Added a unit test in TestDecompression.cc. Verified the issue described in the JIRA is resolved.

…ompressed chunk

dongjoon-hyun

Thank you, @stiga-huang .

c++/src/Compression.cc

c++/test/TestDecompression.cc

dongjoon-hyun · 2022-01-12T07:47:33Z

cc @wgtmac and @williamhyun

dongjoon-hyun · 2022-01-12T07:48:18Z

According to @stiga-huang 's investigation result, I added a milestone 1.7.3 label on this PR.

csringhofer

lgtm, some comments about comments

csringhofer · 2022-01-12T08:57:53Z

c++/src/Compression.cc

@@ -533,24 +545,37 @@ DIAGNOSTIC_PUSH
  }

  /** There are three possible scenarios when seeking a position:


now it is four possible scenarios

c++/src/Compression.cc

csringhofer · 2022-01-12T09:48:19Z

c++/src/Compression.cc

+        outputBuffer = outputBufferStart + posInChunk;
+        return;
+      }
+      // Case 2: The position is outside the decompressed buffer. Skip bytes to seek.


This is a bit confusing as it can only happen in the uncompressed case.

Updated the method comments about this.

csringhofer · 2022-01-12T09:55:20Z

c++/src/Compression.cc

+   *    read yet.
+   * 3. It is already read from the input stream, but has not been decompressed yet, ie.
+   *    it's not in the output stream.
+   * 4. It is not read yet from the input stream.
   */
  void DecompressionStream::seek(PositionProvider& position) {
    size_t seekedPosition = position.current();


not really related to the change, but I think that it would be clearer if this would be renamed, e.g. to startOfSeekedChunk

Yeah, this confused me too. I renamed it to seekedHeaderPosition so it's similar to headerPosition.

wgtmac · 2022-01-12T15:23:43Z

c++/src/Compression.cc

-   * 3. It is not read yet from the inputstream.
+   * 1. The chunk of the seeked position is already read and decompressed into the output
+   *    stream, ie. chunk header is read and chunk contents are in the output stream.
+   * 2. The chunk of the seeked position is partially read. This only happens for


The root cause is in line 495 where DecompressionStream::Next only reads availableSize but not the full uncompressed chunk. So another fix is to enforce read full uncompressed chunk there.

Yeah, I also thought about that but finally chose this fix for simplicity. Loading the whole uncompressed chunk at once might also improve the performance, we can do it in follow-up tasks. :)

c++/src/Compression.cc

dongjoon-hyun · 2022-01-16T00:34:26Z

Gentle ping, @stiga-huang .

stiga-huang · 2022-01-17T03:19:23Z

Sorry to be late here. Thank you guys for the feekback! It also took me some time to understand the existing codes. So I updated some more comments to improve the readability. Please let me know if we need to update more.

wgtmac

LGTM. Thanks @stiga-huang !

dongjoon-hyun

+1, LGTM. Thank you, @stiga-huang , @wgtmac , and csringhofer .
Merged to main/1.7.

…ompressed chunk (#1008) ### What changes were proposed in this pull request? This PR fixes an unhandled case when seeking in an uncompressed chunk. ### Why are the changes needed? The bug causes position overflow and fails the reader when encountered the unhandled case. Some background: * Compressed streams are compressed in chunks. If the compressed size of a chunk is larger than the original size, the original (uncompressed) chunk will be kept. The chunk header records the chunk length and whether it's compressed. * Seek position in a compressed stream is encoded into 2 numbers: pos in the input stream and pos in the chunk. The first number locates the chunk header. The second number locates the position in the decompressed chunk. * Compressed chunks are decompressed in a whole so the whole chunk is in the output buffer. Uncompressed chunks don't need decompression so the input buffer is used directly. However, the chunk could be read in pieces depending on the block size of the input stream. So the seek position might not be loaded yet. The unhandled case is: the seek position is in the current chunk but posInChunk is not loaded yet. We should skip the remaining bytes to seek to it. ### How was this patch tested? Added a unit test in TestDecompression.cc. Verified the issue described in the JIRA is resolved. (cherry picked from commit d175525) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…ompressed chunk (apache#1008) ### What changes were proposed in this pull request? This PR fixes an unhandled case when seeking in an uncompressed chunk. ### Why are the changes needed? The bug causes position overflow and fails the reader when encountered the unhandled case. Some background: * Compressed streams are compressed in chunks. If the compressed size of a chunk is larger than the original size, the original (uncompressed) chunk will be kept. The chunk header records the chunk length and whether it's compressed. * Seek position in a compressed stream is encoded into 2 numbers: pos in the input stream and pos in the chunk. The first number locates the chunk header. The second number locates the position in the decompressed chunk. * Compressed chunks are decompressed in a whole so the whole chunk is in the output buffer. Uncompressed chunks don't need decompression so the input buffer is used directly. However, the chunk could be read in pieces depending on the block size of the input stream. So the seek position might not be loaded yet. The unhandled case is: the seek position is in the current chunk but posInChunk is not loaded yet. We should skip the remaining bytes to seek to it. ### How was this patch tested? Added a unit test in TestDecompression.cc. Verified the issue described in the JIRA is resolved. (cherry picked from commit d175525) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…ompressed chunk (apache#1008) ### What changes were proposed in this pull request? This PR fixes an unhandled case when seeking in an uncompressed chunk. ### Why are the changes needed? The bug causes position overflow and fails the reader when encountered the unhandled case. Some background: * Compressed streams are compressed in chunks. If the compressed size of a chunk is larger than the original size, the original (uncompressed) chunk will be kept. The chunk header records the chunk length and whether it's compressed. * Seek position in a compressed stream is encoded into 2 numbers: pos in the input stream and pos in the chunk. The first number locates the chunk header. The second number locates the position in the decompressed chunk. * Compressed chunks are decompressed in a whole so the whole chunk is in the output buffer. Uncompressed chunks don't need decompression so the input buffer is used directly. However, the chunk could be read in pieces depending on the block size of the input stream. So the seek position might not be loaded yet. The unhandled case is: the seek position is in the current chunk but posInChunk is not loaded yet. We should skip the remaining bytes to seek to it. ### How was this patch tested? Added a unit test in TestDecompression.cc. Verified the issue described in the JIRA is resolved.

ORC-1087: [C++] Handle unloaded seek positions when seeking in an unc…

975f852

…ompressed chunk

github-actions bot added the CPP label Jan 11, 2022

dongjoon-hyun reviewed Jan 11, 2022

View reviewed changes

c++/src/Compression.cc Show resolved Hide resolved

dongjoon-hyun reviewed Jan 11, 2022

View reviewed changes

c++/test/TestDecompression.cc Outdated Show resolved Hide resolved

dongjoon-hyun added this to the 1.7.3 milestone Jan 12, 2022

Rename methods

7806d34

csringhofer approved these changes Jan 12, 2022

View reviewed changes

wgtmac reviewed Jan 12, 2022

View reviewed changes

Update comments

3d7ffba

Fix compile error

6db8ba3

wgtmac approved these changes Jan 17, 2022

View reviewed changes

dongjoon-hyun approved these changes Jan 18, 2022

View reviewed changes

dongjoon-hyun merged commit d175525 into apache:main Jan 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ORC-1087: [C++] Handle unloaded seek positions when seeking in an uncompressed chunk #1008

ORC-1087: [C++] Handle unloaded seek positions when seeking in an uncompressed chunk #1008

stiga-huang commented Jan 11, 2022

dongjoon-hyun left a comment

dongjoon-hyun commented Jan 12, 2022

dongjoon-hyun commented Jan 12, 2022 •

edited

csringhofer left a comment

csringhofer Jan 12, 2022

stiga-huang Jan 17, 2022

csringhofer Jan 12, 2022

stiga-huang Jan 17, 2022

csringhofer Jan 12, 2022

stiga-huang Jan 17, 2022

wgtmac Jan 12, 2022

stiga-huang Jan 17, 2022

dongjoon-hyun commented Jan 16, 2022

stiga-huang commented Jan 17, 2022

wgtmac left a comment

dongjoon-hyun left a comment

		@@ -533,24 +545,37 @@ DIAGNOSTIC_PUSH
		}

		/** There are three possible scenarios when seeking a position:

ORC-1087: [C++] Handle unloaded seek positions when seeking in an uncompressed chunk #1008

ORC-1087: [C++] Handle unloaded seek positions when seeking in an uncompressed chunk #1008

Conversation

stiga-huang commented Jan 11, 2022

What changes were proposed in this pull request?

Why are the changes needed?

How was this patch tested?

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Jan 12, 2022

dongjoon-hyun commented Jan 12, 2022 • edited

csringhofer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Jan 16, 2022

stiga-huang commented Jan 17, 2022

wgtmac left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Jan 12, 2022 •

edited