etseidl commented on code in PR #9967:
URL: https://github.com/apache/arrow-rs/pull/9967#discussion_r3237952470
##########
parquet/src/arrow/arrow_writer/levels.rs:
##########
@@ -645,17 +651,36 @@ impl LevelInfoBuilder {
match &info.logical_nulls {
Some(nulls) => {
assert!(range.end <= nulls.len());
- let nulls = nulls.inner();
- info.def_levels.extend_from_iter(range.clone().map(|i| {
- // Safety: range.end was asserted to be in bounds
earlier
- let valid = unsafe { nulls.value_unchecked(i) };
- max_def_level - (!valid as i16)
- }));
- info.non_null_indices.reserve(len);
- info.non_null_indices.extend(
- BitIndexIterator::new(nulls.inner(), nulls.offset() +
range.start, len)
- .map(|i| i + range.start),
- );
+ // Bulk-fill is profitable only on null-heavy ranges long
enough to
+ // amortize the slice/popcount cost; see
`BULK_FILL_MIN_LEN` and the
+ // PR description for the threshold sweep. The gate uses
the cached
+ // buffer-wide `null_count` (O(1)) to stay cheap on the
cold path.
+ if len >= BULK_FILL_MIN_LEN && nulls.null_count() * 2 >=
nulls.len() {
+ let range_nulls = nulls.slice(range.start, len);
+ let valid_in_range = len - range_nulls.null_count();
Review Comment:
This can move up out of the `if` block...
##########
parquet/src/arrow/arrow_writer/levels.rs:
##########
@@ -645,17 +651,36 @@ impl LevelInfoBuilder {
match &info.logical_nulls {
Some(nulls) => {
assert!(range.end <= nulls.len());
- let nulls = nulls.inner();
- info.def_levels.extend_from_iter(range.clone().map(|i| {
- // Safety: range.end was asserted to be in bounds
earlier
- let valid = unsafe { nulls.value_unchecked(i) };
- max_def_level - (!valid as i16)
- }));
- info.non_null_indices.reserve(len);
- info.non_null_indices.extend(
- BitIndexIterator::new(nulls.inner(), nulls.offset() +
range.start, len)
- .map(|i| i + range.start),
- );
+ // Bulk-fill is profitable only on null-heavy ranges long
enough to
+ // amortize the slice/popcount cost; see
`BULK_FILL_MIN_LEN` and the
+ // PR description for the threshold sweep. The gate uses
the cached
+ // buffer-wide `null_count` (O(1)) to stay cheap on the
cold path.
+ if len >= BULK_FILL_MIN_LEN && nulls.null_count() * 2 >=
nulls.len() {
+ let range_nulls = nulls.slice(range.start, len);
+ let valid_in_range = len - range_nulls.null_count();
+ let null_def_level = max_def_level - 1;
+ let buf = info
+ .def_levels
+ .materialize_mut()
+ .expect("definition levels present");
+ let base = buf.len();
+ buf.resize(base + len, null_def_level);
+ for i in range_nulls.valid_indices() {
+ buf[base + i] = max_def_level;
+ }
+ info.non_null_indices.reserve(valid_in_range);
+ info.non_null_indices
+ .extend(range_nulls.valid_indices().map(|i| i +
range.start));
+ } else {
+ let range_nulls = nulls.slice(range.start, len);
Review Comment:
then remove this...
##########
parquet/src/arrow/arrow_writer/levels.rs:
##########
Review Comment:
perhaps `info.non_null_indices.reserve(len)` here?
##########
parquet/src/arrow/arrow_writer/levels.rs:
##########
@@ -645,17 +651,36 @@ impl LevelInfoBuilder {
match &info.logical_nulls {
Some(nulls) => {
assert!(range.end <= nulls.len());
- let nulls = nulls.inner();
- info.def_levels.extend_from_iter(range.clone().map(|i| {
- // Safety: range.end was asserted to be in bounds
earlier
- let valid = unsafe { nulls.value_unchecked(i) };
- max_def_level - (!valid as i16)
- }));
- info.non_null_indices.reserve(len);
- info.non_null_indices.extend(
- BitIndexIterator::new(nulls.inner(), nulls.offset() +
range.start, len)
- .map(|i| i + range.start),
- );
+ // Bulk-fill is profitable only on null-heavy ranges long
enough to
+ // amortize the slice/popcount cost; see
`BULK_FILL_MIN_LEN` and the
+ // PR description for the threshold sweep. The gate uses
the cached
+ // buffer-wide `null_count` (O(1)) to stay cheap on the
cold path.
+ if len >= BULK_FILL_MIN_LEN && nulls.null_count() * 2 >=
nulls.len() {
+ let range_nulls = nulls.slice(range.start, len);
+ let valid_in_range = len - range_nulls.null_count();
+ let null_def_level = max_def_level - 1;
+ let buf = info
+ .def_levels
+ .materialize_mut()
+ .expect("definition levels present");
+ let base = buf.len();
+ buf.resize(base + len, null_def_level);
+ for i in range_nulls.valid_indices() {
+ buf[base + i] = max_def_level;
+ }
+ info.non_null_indices.reserve(valid_in_range);
+ info.non_null_indices
+ .extend(range_nulls.valid_indices().map(|i| i +
range.start));
+ } else {
+ let range_nulls = nulls.slice(range.start, len);
+ info.def_levels.extend_from_iter(
+ range_nulls
+ .iter()
+ .map(|valid| max_def_level - (!valid as i16)),
+ );
+ info.non_null_indices
+ .extend(range_nulls.valid_indices().map(|i| i +
range.start));
Review Comment:
and remove this (seems a `reserve` went missing as well).
##########
parquet/src/arrow/arrow_writer/levels.rs:
##########
@@ -645,17 +651,36 @@ impl LevelInfoBuilder {
match &info.logical_nulls {
Some(nulls) => {
assert!(range.end <= nulls.len());
- let nulls = nulls.inner();
- info.def_levels.extend_from_iter(range.clone().map(|i| {
- // Safety: range.end was asserted to be in bounds
earlier
- let valid = unsafe { nulls.value_unchecked(i) };
- max_def_level - (!valid as i16)
- }));
- info.non_null_indices.reserve(len);
- info.non_null_indices.extend(
- BitIndexIterator::new(nulls.inner(), nulls.offset() +
range.start, len)
- .map(|i| i + range.start),
- );
+ // Bulk-fill is profitable only on null-heavy ranges long
enough to
+ // amortize the slice/popcount cost; see
`BULK_FILL_MIN_LEN` and the
+ // PR description for the threshold sweep. The gate uses
the cached
+ // buffer-wide `null_count` (O(1)) to stay cheap on the
cold path.
+ if len >= BULK_FILL_MIN_LEN && nulls.null_count() * 2 >=
nulls.len() {
+ let range_nulls = nulls.slice(range.start, len);
+ let valid_in_range = len - range_nulls.null_count();
+ let null_def_level = max_def_level - 1;
+ let buf = info
+ .def_levels
+ .materialize_mut()
+ .expect("definition levels present");
+ let base = buf.len();
+ buf.resize(base + len, null_def_level);
+ for i in range_nulls.valid_indices() {
+ buf[base + i] = max_def_level;
+ }
+ info.non_null_indices.reserve(valid_in_range);
+ info.non_null_indices
+ .extend(range_nulls.valid_indices().map(|i| i +
range.start));
Review Comment:
and this can move down out of the `if` block
--
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]
