[GitHub] [orc] csringhofer commented on a change in pull request #1008: ORC-1087: [C++] Handle unloaded seek positions when seeking in an uncompressed chunk

[GitBox <gi...@apache.org>]

csringhofer commented on a change in pull request #1008:
URL: https://github.com/apache/orc/pull/1008#discussion_r782839420



##########
File path: c++/src/Compression.cc
##########
@@ -533,24 +545,37 @@ DIAGNOSTIC_PUSH
   }
 
   /** There are three possible scenarios when seeking a position:

Review comment:
       now it is four possible scenarios

##########
File path: c++/src/Compression.cc
##########
@@ -533,24 +545,37 @@ DIAGNOSTIC_PUSH
   }
 
   /** There are three possible scenarios when seeking a position:
-   * 1. The seeked position is already read and decompressed into
-   *    the output stream.
-   * 2. It is already read from the input stream, but has not been
-   *    decompressed yet, ie. it's not in the output stream.
-   * 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
+   *    uncompressed chunks. The chunk header is read but the seeked position hasn't been
+   *    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();

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

##########
File path: c++/src/Compression.cc
##########
@@ -533,24 +545,37 @@ DIAGNOSTIC_PUSH
   }
 
   /** There are three possible scenarios when seeking a position:
-   * 1. The seeked position is already read and decompressed into
-   *    the output stream.
-   * 2. It is already read from the input stream, but has not been
-   *    decompressed yet, ie. it's not in the output stream.
-   * 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
+   *    uncompressed chunks. The chunk header is read but the seeked position hasn't been
+   *    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();
-    // Case 1: the seeked position is the one that is currently buffered and
-    // decompressed. Here we only need to set the output buffer's pointer to the
-    // seeked position. Note that after the headerPosition comes the 3 bytes of
-    // the header.
+    // Case 1&2: the seeked position is in the current chunk and it's buffered and
+    // decompressed. Note that after the headerPosition comes the 3 bytes of the header.
     if (headerPosition == seekedPosition
         && inputBufferStartPosition <= headerPosition + 3 && inputBufferStart) {
       position.next(); // Skip the input level position.
       size_t posInChunk = position.next(); // Chunk level position.
-      outputBufferLength = uncompressedBufferLength - posInChunk;
-      outputBuffer = outputBufferStart + posInChunk;
+      // Case 1: The position is in the decompressed buffer. Here we only need to
+      // set the output buffer's pointer to the seeked position.
+      if (uncompressedBufferLength >= posInChunk) {
+        outputBufferLength = uncompressedBufferLength - posInChunk;
+        outputBuffer = outputBufferStart + posInChunk;
+        return;
+      }
+      // Case 2: The position is outside the decompressed buffer. Skip bytes to seek.

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

##########
File path: c++/src/Compression.cc
##########
@@ -321,6 +321,17 @@ DIAGNOSTIC_PUSH
                          DECOMPRESS_ORIGINAL,
                          DECOMPRESS_EOF};
 
+  std::string decompressStateToString(DecompressState state) {

Review comment:
       I couldn't find the place where we use this function




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org