Skip to content

ARROW-11063: [Rust] [Breaking] Validate null counts when building arrays#9041

Closed
nevi-me wants to merge 1 commit into
apache:masterfrom
nevi-me:ARROW-11063
Closed

ARROW-11063: [Rust] [Breaking] Validate null counts when building arrays#9041
nevi-me wants to merge 1 commit into
apache:masterfrom
nevi-me:ARROW-11063

Conversation

@nevi-me

@nevi-me nevi-me commented Dec 29, 2020

Copy link
Copy Markdown
Contributor

Removes the null_count() function, forcing the builder to always validate the null count.

This breaks existing code that provides null counts, with the required action being to remove that code.

Removes the null_count() function, forcing the builder to always validate the null count
@github-actions

Copy link
Copy Markdown

@jorgecarleitao jorgecarleitao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The idea is good, and the code looks good. Thanks @nevi-me !

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This also looks good to me -- nice work @nevi-me !

@alamb alamb closed this in 30ce2eb Dec 29, 2020
@nevi-me

nevi-me commented Dec 29, 2020

Copy link
Copy Markdown
Contributor Author

There's a rustfmt lint that's failed on my CI run, I didn't see it till now. I'm using those new macbooks while I'm away from home, so I don't have the stable toolchain yet.

May someone who works on the next PR, fix the error for me :)

@nevi-me nevi-me deleted the ARROW-11063 branch December 29, 2020 23:34
@mqy

mqy commented Dec 30, 2020

Copy link
Copy Markdown
Contributor

@nevi-me format in this way should avoid the problem

I have fixed it in #9025

Also I'm thinking about add cargo +nightly-2020-11-24-x86_64 fmt to pre-commit.sh

@alamb

alamb commented Dec 30, 2020

Copy link
Copy Markdown
Contributor

🤦 Sorry @nevi-me -- I also have a PR here to fix it: #9046

I think it is blocking other PRs so I will merge #9046 as soon as it is green. cc @mqy -- thank you

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants