Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Array::with_nulls #6659

Closed
wants to merge 1 commit into from
Closed

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Oct 31, 2024

Which issue does this PR close?

Closes #6528

Rationale for this change

Propose adding a new safe API to replace nulls

@tustvold mentioned the potential implications of this API #6528 (comment) here. I added an example to show what was going on

What changes are included in this PR?

Add Array::with_nulls

Are there any user-facing changes?

New API

@github-actions github-actions bot added the arrow Changes to the arrow crate label Oct 31, 2024
///
/// # Example:
/// ```
/// # use arrow_array::{Array, Int32Array};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the new API and an example. I am not sure this is a good idea due to @tustvold 's comment here: #6528 (comment) but figured I would make a PR for discussion (and only now do I really understand what he is talking about)

/// let array_with_nulls = array.with_nulls(nulls);
/// assert_eq!(
/// array_with_nulls.as_primitive(),
/// &Int32Array::from(vec![None, Some(0), None, Some(4), None]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the fact that this is now Some(0) (whatever value was created for the null element in the original array) could have potential issues 🤔

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

As written this is unsound, in particular it allows unmasking indexes within dictionaries that aren't valid.

I think the only reasonable behaviour is to combine the null masks, at which point this is just a special case of the nullif kernel.

@alamb
Copy link
Contributor Author

alamb commented Oct 31, 2024

I agree -- let's not do this. I have another idea that might help #6528 (comment)

@alamb alamb closed this Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Safe API to replace NullBuffers for Arrays
2 participants