AntoinePrv commented on code in PR #47294: URL: https://github.com/apache/arrow/pull/47294#discussion_r2349510714
########## cpp/src/arrow/util/rle_encoding_internal.h: ########## @@ -84,32 +85,278 @@ namespace util { /// (total 26 bytes, 1 byte overhead) // +class RleRun { + public: + using byte = uint8_t; + /// Enough space to store a 64bit value + using raw_data_storage = std::array<byte, 8>; + using raw_data_const_pointer = const byte*; + using raw_data_size_type = int32_t; + /// The type of the size of either run, between 1 and 2^31-1 as per Parquet spec + using values_count_type = int32_t; + /// The type to represent a size in bits + using bit_size_type = int32_t; + + constexpr RleRun() noexcept = default; Review Comment: Looking only at the Run classes, they do feel like they could be simple classes. However, in the much other functions in this file, they bring a much welcomed (small) reduction of complexity: - The two run classes have a similar interface; - They manage the conversion from bit size and number of values to a ready to use ptr+len; - `RleRun` hides the unused bytes. Perhaps we could instead put the member functions definition within the class (as suggested by another comment). -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org