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.

Sign up for GitHub

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

Jump to bottom

Fix some clippy lints in parquet crate, rename LevelEncoder variants to conform to Rust standards #1273

Merged
merged 16 commits into from
Feb 8, 2022
Merged

Fix some clippy lints in parquet crate, rename LevelEncoder variants to conform to Rust standards #1273

merged 16 commits into from
Feb 8, 2022

Conversation

HaoYang670
Copy link
Contributor

@HaoYang670 HaoYang670 commented Feb 5, 2022 •
edited
Loading

Which issue does this PR close?

Re #1254. Partially close it

Rationale for this change

In this PR, we remove some disabled clippy lints and refactor the code, which includes:

  • vec_init_then_push
  • upper_case_acronyms
  • transmute_ptr_ti_ptr
  • same_item_push
  • approx_constant
  • cast_ptr_alignment
  • float_cmp
  • float_equality_without_abs
  • incomplete_features
  • single_char_names
  • needless_range_loop

What changes are included in this PR?

Are there any user-facing changes?

Some enums are renamed.

@HaoYang670
disallow vec_init_then_push
7f1505e 
Signed-off-by: remzi <[email protected]>
@HaoYang670
disallow upper case acronyms
e169ec6 
Signed-off-by: remzi <[email protected]>
@HaoYang670
disallow transmute ptr to ptr
92cd77c 
Signed-off-by: remzi <[email protected]>
@HaoYang670
disallow same item push
b87bac6 
Signed-off-by: remzi <[email protected]>
@HaoYang670
disallow approx constant
acb6333 
Signed-off-by: remzi <[email protected]>
@HaoYang670
disallow cast ptr alignment
5bd293d 
Signed-off-by: remzi <[email protected]>
@HaoYang670
disallow float cmp
1f0aa89 
Signed-off-by: remzi <[email protected]>
@HaoYang670
check float equality with abs
d6d335e 
Signed-off-by: remzi <[email protected]>
@HaoYang670
check incomplete features
48384ac 
Signed-off-by: remzi <[email protected]>
@HaoYang670
check single char names
0ef417d 
Signed-off-by: remzi <[email protected]>
@HaoYang670
check needless range loop
20f4237 
Signed-off-by: remzi <[email protected]>
@github-actions github-actions bot added the parquet Changes to the parquet crate label Feb 5, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 5, 2022 •
edited
Loading

Codecov Report

Merging #1273 (e569ea3) into master (ce15d0c) will decrease coverage by 0.00%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1273      +/-   ##
==========================================
- Coverage   83.02%   83.02%   -0.01%     
==========================================
  Files         180      180              
  Lines       52269    52262       -7     
==========================================
- Hits        43394    43388       -6     
+ Misses       8875     8874       -1
Impacted Files Coverage Δ
parquet/src/record/reader.rs 89.83% <ø> (ø)
parquet/src/arrow/record_reader.rs 93.43% <66.66%> (-0.65%) ⬇️
parquet/src/encodings/levels.rs 93.41% <94.73%> (+0.36%) ⬆️
parquet/src/arrow/schema.rs 85.56% <100.00%> (ø)
parquet/src/data_type.rs 76.49% <100.00%> (-0.12%) ⬇️
parquet/src/encodings/decoding.rs 90.45% <100.00%> (ø)
parquet/src/encodings/encoding.rs 93.71% <100.00%> (+0.19%) ⬆️
parquet/src/encodings/rle.rs 92.62% <100.00%> (-0.08%) ⬇️
parquet/src/file/writer.rs 93.30% <100.00%> (ø)
parquet/src/record/api.rs 91.65% <100.00%> (ø)
... and 6 more

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 ce15d0c...e569ea3. Read the comment docs.

@HaoYang670 HaoYang670 marked this pull request as draft February 5, 2022 06:01
@alamb
Copy link
Contributor

alamb commented Feb 7, 2022

@HaoYang670 this PR is marked "draft" - is it ready for review?

@HaoYang670 HaoYang670 marked this pull request as ready for review February 8, 2022 01:25
@HaoYang670
Copy link
Contributor Author

Yes, ready for review.

alamb
alamb approved these changes Feb 8, 2022
View reviewed changes
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.

Thank you @HaoYang670

Looks really nice to me ❤️

parquet/src/encodings/decoding.rs
if i % 8 == 0 {
v.push(0);
}
if data[i] {
if *item {
Copy link
Contributor

Choose a reason for hiding this comment

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

this type of change is not only more "idiomatic" I think it will also be faster (as bounds checks aren't being done on each access of data 👍 )

parquet/src/encodings/levels.rs
@@ -50,11 +50,11 @@ pub fn max_buffer_size(
}

/// Encoder for definition/repetition levels.
/// Currently only supports RLE and BIT_PACKED (dev/null) encoding, including v2.
/// Currently only supports Rle and BitPacked (dev/null) encoding, including v2.
pub enum LevelEncoder {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically a breaking API change, but I think it is ok

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @tustvold

Copy link
Contributor

Choose a reason for hiding this comment

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

The encodings module is experimental and so this isn't actually a breaking change. Clippy ignores proposing breaking changes to the API (at least for this lint because the salt was strong).

parquet/src/lib.rs
@@ -30,24 +30,13 @@
//!
//! 3. [arrow::async_reader] for `async` reading and writing parquet
//! files to Arrow `RecordBatch`es (requires the `async` feature).
#![allow(incomplete_features)]
#![allow(dead_code)]
#![allow(non_camel_case_types)]
#![allow(
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@alamb alamb added the api-change Changes to the arrow API label Feb 8, 2022
@alamb alamb changed the title Fix some clippy lints in parquet crate Fix some clippy lints in parquet crate, rename LevelEncoder variants to conform to Rust standards Feb 8, 2022
@alamb alamb merged commit dbc1752 into apache:master Feb 8, 2022
@alamb alamb removed the api-change Changes to the arrow API label Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

@tustvold tustvold tustvold left review comments

Assignees
No one assigned
Labels
parquet Changes to the parquet crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HaoYang670 @codecov-commenter @alamb @tustvold