Skip to content

Reduce duplication when computing nulls#17

Open
alamb wants to merge 1 commit into
Dandandan:coalesce_batches_filterfrom
alamb:alamb/reduce_duplication
Open

Reduce duplication when computing nulls#17
alamb wants to merge 1 commit into
Dandandan:coalesce_batches_filterfrom
alamb:alamb/reduce_duplication

Conversation

@alamb

@alamb alamb commented Jan 19, 2026

Copy link
Copy Markdown

This targets this PR from @Dandandan

I noticed that there was some duplicated code, and actually one path was not consistent

let's calculate the nulls to use once

@github-actions github-actions Bot added the arrow label Jan 19, 2026
.slice(offset, len);
let s = s.as_primitive::<T>();

let nulls = s.nulls().filter(|n| n.null_count() > 0);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By calculating this one rather than replicating it in each branch, it reduces code and ensures it is consistent (actually the last branch did not filter on null_count)

// Copy all values
self.current.extend_from_slice(values);
if let Some(nulls) = s.nulls() {
if let Some(nulls) = nulls {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was not consistent with the other branches (it didn't previously check for null count)

However II am pretty sure this code is unreachable 🤷

) -> Result<(), ArrowError> {
// We only support primitve now, fallback to filter_record_batch for other types
// Also, skip optimization when filter is not very selectivex§
// We only support primitive now, fallback to filter_record_batch for other types

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drive by comment cleanup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant