[GitHub] [arrow] felipecrv commented on a diff in pull request #35677: GH-35675 [C++] Don't copy the ArraySpan into the REE ArraySpan

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

felipecrv commented on code in PR #35677:
URL: https://github.com/apache/arrow/pull/35677#discussion_r1199003065


##########
cpp/src/arrow/util/ree_util.h:
##########
@@ -227,20 +227,39 @@ class RunEndEncodedArraySpan {
     int64_t physical_pos_;
   };
 
-  explicit RunEndEncodedArraySpan(const ArrayData& data)
-      : RunEndEncodedArraySpan(ArraySpan{data}) {}
+  // Prevent implicit ArrayData -> ArraySpan conversion in
+  // RunEndEncodedArraySpan instantiation.
+  explicit RunEndEncodedArraySpan(const ArrayData& data) = delete;
 
-  explicit RunEndEncodedArraySpan(const ArraySpan& array_span)
-      : array_span{array_span}, run_ends_(RunEnds<RunEndCType>(array_span)) {
-    assert(array_span.type->id() == Type::RUN_END_ENCODED);
+  /// \brief Construct a RunEndEncodedArraySpan from an ArraySpan and new
+  /// absolute offset and length.
+  ///
+  /// RunEndEncodedArraySpan{span, off, len} is equivalent to:
+  ///
+  ///   span.SetSlice(off, len);
+  ///   RunEndEncodedArraySpan{span}
+  ///
+  /// ArraySpan::SetSlice() updates the null_count to kUnknownNullCount, but

Review Comment:
   That improvement seems unrelated to this change and affects code that is much more widely used. Could you create an issue for that? My comment here is just an informal proof that the equivalence here holds even though I don't perform all the operations that `SetSlice` does based on my class only taking ArraySpans of REEs.



-- 
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-unsubscribe@arrow.apache.org

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