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

ARROW-8597 [Rust] Lints and readability improvements for arrow crate #7042

Closed
wants to merge 3 commits into from

Conversation

durch
Copy link
Contributor

@durch durch commented Apr 26, 2020

  • Pedantic fixes to unsafe
  • Changes to function arguments to pass in references or values as appropriate
  • Refactor pointer arithmetic to use usize instead of isize casting
  • Ignore generated code clippy warnings
  • Refactor loops to use iterators where appropriate
  • Remove unnecessary return statements
  • Remove unnecessary clone statements
  • Remove unnecessary closures
  • A bunch of similar small scale refactorings

Tests below are currently failing on master locally as well with ARROW_TEST_DATA not defined: NotPresent, assuming that is ok for now:

ipc::reader::tests::read_generated_files
ipc::reader::tests::read_generated_streams
ipc::writer::tests::read_and_rewrite_generated_files
ipc::writer::tests::read_and_rewrite_generated_streams

@github-actions
Copy link

Copy link
Contributor

@paddyhoran paddyhoran left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @durch

One minor nit.

Looks like the lint check is failing CI. Once these are fixed we can get this merged.

rust/arrow/src/util/bit_util.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

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

Thanks @durch, I didn't see a clippy ignore for len() without is_empty(), or did you ignore that lint. I had recently come across it, think that is_empty() wouldn't be useful as we rarely have empty arrays.

@durch
Copy link
Contributor Author

durch commented Apr 27, 2020

@nevi-me I left it as is for now, did not want to actually write any new code in this PR. After this one gets merged I'll make another pass and write some code, in addition to the is_empty there is also a low hanging Default impl, possibly a few more pedantic changes :).

@markhildreth
Copy link
Contributor

@durch Re: "ARROW_TEST_DATA not defined", the /rust/README.md file in the repo has some additional steps needed to get access to those test files locally. Basically...

git submodule update --init

...then run cargo test with these environment variables set:

PARQUET_TEST_DATA=/path/to/arrow/cpp/submodules/parquet-testing/data
ARROW_TEST_DATA=/path/to/arrow/testing/data/

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.

None yet

4 participants