mbutrovich commented on code in PR #3659:
URL: https://github.com/apache/datafusion-comet/pull/3659#discussion_r3267383805
##########
native/shuffle/src/spark_unsafe/list.rs:
##########
@@ -45,40 +45,43 @@ macro_rules! impl_append_to_builder {
return;
}
+ // SAFETY: element_offset points to contiguous element data of
length num_elements.
+ debug_assert!(self.element_offset != 0, "element_offset is null");
+ let ptr = self.element_offset as *const $element_type;
+ let aligned = (ptr as
usize).is_multiple_of(std::mem::align_of::<$element_type>());
Review Comment:
The "alignment is guaranteed by SparkUnsafeArray layout" line in the PR body
reads as though this runtime check is redundant, but I think it is actually
load-bearing for soundness. `unsafe_object.rs:49` and the existing comment at
`list.rs:105` both say the array base can be unaligned when nested in a row's
variable-length region, and `std::slice::from_raw_parts::<T>` on a misaligned
pointer is UB. Could we keep the check (as you have done here) but reword the
PR description so a future reader does not delete it on the assumption that
alignment is guaranteed?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
