[GitHub] [orc] kaka11chen commented on a diff in pull request #1469: ORC-1408: [C++] Add comments and test for row batch not reset if strip doesn't have PRESENT stream.

["kaka11chen (via GitHub)" <gi...@apache.org>]

kaka11chen commented on code in PR #1469:
URL: https://github.com/apache/orc/pull/1469#discussion_r1179895538


##########
c++/include/orc/Vector.hh:
##########
@@ -37,6 +37,8 @@ namespace orc {
    * The base class for each of the column vectors. This class handles
    * the generic attributes such as number of elements, capacity, and
    * notNull vector.
+   * Note: Because we don't reset notNull buffer if hasNulls == false for better performance,
+   * it need to check hasNull firstly, and then check notNull buffer.

Review Comment:
   > The new comment seems to be reader-oriented. We'd better rephrase it to describe the contract, something like:
   > 
   > ```
   > Note: If hasNull is true, the values in the notNull buffer are not required.
   > On the writer side, it does not read values from notNull buffer so users are
   > not expected to write notNull buffer if hasNull is true. On the reader side,
   > it does not set notNull buffer if hasNull is true, meaning that it is undefined
   > behavior to consume values from notNull buffer in this case by downstream users.
   > ```
   
   Thanks, it is better, I have updated.



-- 
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: issues-unsubscribe@orc.apache.org

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