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

Support casting to/from DataType::Null in cast kernel #1572

Merged
merged 3 commits into from
Apr 18, 2022

Conversation

WinkerDu
Copy link
Contributor

@WinkerDu WinkerDu commented Apr 15, 2022

Which issue does this PR close?

Closes #684.

Rationale for this change

Casting from DataType::Null to many other types is not allowed for now, like DataType::Null <-> DataType::Utf8, apache/arrow-datafusion#1188 depends on this casting ability to solve NULL constant issue in function call

What changes are included in this PR?

Allow casting DataType::Null from and to most of types except for DataType::Union and DataType::Decimal, which is not supported in

pub fn new_null_array(data_type: &DataType, length: usize) -> ArrayRef {

Are there any user-facing changes?

No.

@github-actions github-actions bot added the arrow Changes to the arrow crate label Apr 15, 2022
@WinkerDu
Copy link
Contributor Author

WinkerDu commented Apr 15, 2022

@alamb @Dandandan plz have a review, thank you.

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2022

Codecov Report

Merging #1572 (b850e3e) into master (5dca6f0) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1572      +/-   ##
==========================================
+ Coverage   82.87%   82.93%   +0.05%     
==========================================
  Files         193      193              
  Lines       55304    55343      +39     
==========================================
+ Hits        45835    45897      +62     
+ Misses       9469     9446      -23     
Impacted Files Coverage Δ
arrow/src/compute/kernels/cast.rs 95.81% <100.00%> (+0.07%) ⬆️
parquet_derive/src/parquet_field.rs 65.75% <0.00%> (-0.69%) ⬇️
arrow/src/array/transform/mod.rs 86.35% <0.00%> (-0.23%) ⬇️
arrow/src/ipc/reader.rs 88.61% <0.00%> (ø)
arrow/src/datatypes/field.rs 54.62% <0.00%> (+0.29%) ⬆️
arrow/src/array/array.rs 92.70% <0.00%> (+7.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dca6f0...b850e3e. Read the comment docs.

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.

This looks good to me, thank you 👍. Left some minor test suggestions

arrow/src/compute/kernels/cast.rs Outdated Show resolved Hide resolved
| LargeList(_)
| FixedSizeList(_, _)
| Struct(_)
| Map(_, _)
| Dictionary(_, _),
Null,
) => Ok(new_null_array(to_type, array.len())),
Copy link
Contributor

Choose a reason for hiding this comment

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

I've confirmed that these types are all supported by new_null_array

arrow/src/compute/kernels/cast.rs Outdated Show resolved Hide resolved
arrow/src/compute/kernels/cast.rs Show resolved Hide resolved
// cast null from and to struct
let data_type =
DataType::Struct(vec![Field::new("data", DataType::Int64, false)]);
cast_from_and_to_null!(data_type);
Copy link
Contributor

Choose a reason for hiding this comment

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

Map?

}

#[test]
fn test_cast_null_from_and_to_variable_sized() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe fixed size also whilst here?

@WinkerDu
Copy link
Contributor Author

WinkerDu commented Apr 16, 2022

@tustvold @alamb
A follow up commit pushed:

  • more test cases added
  • change cast_from_and_to_null from macro to function

Please approve the running workflows and have another review, thank you :)

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.

Even better now 😄

@WinkerDu
Copy link
Contributor Author

WinkerDu commented Apr 17, 2022

cc @alamb @Dandandan @yjshen PTAL, thank you.

@alamb alamb changed the title allow casting DataType::Null from and to others Support casting to/from DataType::Null in cast kernel Apr 17, 2022
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Nice -- thanks @WinkerDu !

@WinkerDu
Copy link
Contributor Author

@alamb @tustvold Is this pr OK to merge,
Or can you help @ some other committers to take another look?
Thank you

@yjshen yjshen merged commit 8bed7ea into apache:master Apr 18, 2022
alamb pushed a commit to alamb/arrow-rs that referenced this pull request Apr 18, 2022
* cast null from and to others

* fmt fix

* add more ut

Co-authored-by: duripeng <[email protected]>
@alamb
Copy link
Contributor

alamb commented Apr 18, 2022

Thanks @yjshen ❤️

WinkerDu added a commit to WinkerDu/arrow-rs that referenced this pull request Apr 23, 2022
* cast null from and to others

* fmt fix

* add more ut

Co-authored-by: duripeng <[email protected]>
@alamb alamb added the enhancement Any new improvement worthy of a entry in the changelog label Apr 27, 2022
MazterQyou pushed a commit to cube-js/arrow-rs that referenced this pull request Jun 9, 2023
* cast null from and to others

* fmt fix

* add more ut

Co-authored-by: duripeng <[email protected]>
MazterQyou pushed a commit to cube-js/arrow-rs that referenced this pull request Jun 9, 2023
* cast null from and to others

* fmt fix

* add more ut

Co-authored-by: duripeng <[email protected]>
MazterQyou pushed a commit to cube-js/arrow-rs that referenced this pull request Jan 19, 2024
* cast null from and to others

* fmt fix

* add more ut

Co-authored-by: duripeng <[email protected]>
MazterQyou pushed a commit to cube-js/arrow-rs that referenced this pull request Feb 5, 2024
* cast null from and to others

* fmt fix

* add more ut

Co-authored-by: duripeng <[email protected]>
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 enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow casting from DataType::Null to any other type
5 participants