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

Minor: make it easier to find fix instructions when cargo fmt on parquet fails #6886

Merged
merged 4 commits into from
Dec 18, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Dec 16, 2024

Which issue does this PR close?

Closes #.

Rationale for this change

While working on #6840 the fmt ci check failed on parquet. For example: https://github.com/apache/arrow-rs/actions/runs/12361198145/job/34497838320

Run cargo fmt -p parquet -- --check --config skip_children=true `find . -name "*.rs" \! -name format.rs`
Diff in /__w/arrow-rs/arrow-rs/parquet/src/arrow/schema/mod.rs:1[7](https://github.com/apache/arrow-rs/actions/runs/12361198145/job/34497838320#step:7:8)75:
             Field::new("decimal256", DataType::Decimal256(39, 2), false),
         ];
         let arrow_schema = Schema::new(arrow_fields);
-        let converted_arrow_schema = ArrowSchemaConverter::new()
-            .convert(&arrow_schema)
-            .unwrap();
+        let converted_arrow_schema = ArrowSchemaConverter::new().convert(&arrow_schema).unwrap();
 
         assert_eq!(
             parquet_schema.columns().len(),
Error: Process completed with exit code 1.

However, the exact recipe to fix this was not clear (you have to run a certain command within the arquet directory)

What changes are included in this PR?

  1. Add the commands needed to fix CI as comments in the script so they are visible in the failure

Example https://github.com/apache/arrow-rs/actions/runs/12361621609/job/34499177568?pr=6886

Screenshot 2024-12-16 at 4 30 04 PM

Are there any user-facing changes?

@github-actions github-actions bot added the parquet Changes to the parquet crate label Dec 16, 2024
@alamb alamb force-pushed the alamb/add_fix_instructions branch from 2c28558 to 3da1765 Compare December 16, 2024 21:31
@github-actions github-actions bot removed the parquet Changes to the parquet crate label Dec 16, 2024
@alamb alamb marked this pull request as ready for review December 16, 2024 21:52
@alamb alamb added the development-process Related to development process of arrow-rs label Dec 16, 2024
Copy link
Contributor

@etseidl etseidl left a comment

Choose a reason for hiding this comment

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

🚀 Nice. Just a small correction.

.github/workflows/rust.yml Outdated Show resolved Hide resolved
Co-authored-by: Ed Seidl <etseidl@users.noreply.github.com>
@alamb
Copy link
Contributor Author

alamb commented Dec 17, 2024

🚀 Nice. Just a small correction.

Nice catch -- 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.

Is there a reason we have such an arcane incantation?

@alamb
Copy link
Contributor Author

alamb commented Dec 17, 2024

Is there a reason we have such an arcane incantation?

Because for some reason cargo fmt doesn't format the files in the parquet crate.

Added in 25e1969 / #6328

@etseidl
Copy link
Contributor

etseidl commented Dec 18, 2024

Is there a reason we have such an arcane incantation?

Because for some reason cargo fmt doesn't format the files in the parquet crate.

Added in 25e1969 / #6328

In particular, things like

arrow-rs/parquet/src/lib.rs

Lines 137 to 138 in 54dccad

experimental!(mod compression);
experimental!(mod encodings);
cause entire modules to be skipped by cargo fmt. I agree the incantation is not desirable, and was meant to be a stop-gap until a better solution can be found.

@tustvold tustvold merged commit 3317989 into apache:main Dec 18, 2024
33 checks passed
@alamb
Copy link
Contributor Author

alamb commented Dec 18, 2024

cause entire modules to be skipped by cargo fmt. I agree the incantation is not desirable, and was meant to be a stop-gap until a better solution can be found.

Filed #6897 to track

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development-process Related to development process of arrow-rs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants