bkietz commented on code in PR #38027:
URL: https://github.com/apache/arrow/pull/38027#discussion_r1394282341


##########
cpp/src/arrow/array/data.h:
##########
@@ -434,6 +434,21 @@ struct ARROW_EXPORT ArraySpan {
     return GetValues<T>(i, this->offset);
   }
 
+  // Access a buffer's data as a span
+  template <typename T>
+  util::span<const T> GetSpan(int i) const {
+    return util::span(buffers[i].data_as<T>(),
+                      static_cast<size_t>(buffers[i].size) / sizeof(T))
+        .subspan(offset);
+  }
+
+  template <typename T>
+  util::span<T> GetSpan(int i) {
+    return util::span(buffers[i].mutable_data_as<T>(),
+                      static_cast<size_t>(buffers[i].size) / sizeof(T))
+        .subspan(offset);

Review Comment:
   I think adding an explicit parameter for length would be fine but maybe 
wouldn't add much value:
   ```diff
   -data.GetSpan<int32_t>(1).subspan(0, 50);
   +data.GetSpan<int32_t>(1, /*length=*/50);
   ```
   
   Maybe instead we could just add a `bool one_extra` parameter? 
   ```diff
   -data.GetSpan<int32_t>(1).subspan(0, 50);
   +data.GetSpan<int32_t>(1, /*one_extra=*/true); // offsets buffer
   ```
   
   Also, it's worth noting that these will (I think) only rarely be used on a 
buffer other than 1; it'll never be correct to use these on a null bitmap and I 
think list view and dense union are the only layouts which have buffers beyond 
1 which aren't raw bytes



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to