Skip to content

Remove lint issues in parquet-related code.#9375

Merged
Jefffrey merged 7 commits intoapache:mainfrom
brunal:main
Feb 13, 2026
Merged

Remove lint issues in parquet-related code.#9375
Jefffrey merged 7 commits intoapache:mainfrom
brunal:main

Conversation

@brunal
Copy link
Contributor

@brunal brunal commented Feb 8, 2026

  • CacheOptions: use in arrow/push_decoder/reader_builder/mod.rs was
    unused. But removing it triggers a doc complaint in array_reader.rs.
    I fix it by spelling out CacheOptions at one usage site.

  • ExtensionType import was unused in arrow/schema/extension.rs when
    compiling with default features. Only use it inside the
    feature-guarded code paths.

  • Prefix unused function arguments with _.

  • Some code in parquet/tests/arrow_reader/io/mod.rs is unused. As I lack
    knowledge of the context, I just add just add #[allow(dead_code)].

Rationale for this change

rust-analyzer bugs me about those unused symbols & co.

@github-actions github-actions bot added the parquet Changes to the parquet crate label Feb 8, 2026
@brunal brunal marked this pull request as draft February 8, 2026 08:50
@brunal brunal force-pushed the main branch 2 times, most recently from 2bb71a6 to efe310e Compare February 9, 2026 09:14
* CacheOptions: `use` in arrow/push_decoder/reader_builder/mod.rs was
  unused. But removing it triggers a doc complaint in array_reader.rs.
  I fix it by spelling out `CacheOptions` at one usage site.

* ExtensionType import was unused in arrow/schema/extension.rs when
  compiling with default features. Only `use` it inside the
  feature-guarded code paths.

* Prefix unused function arguments with `_`.

* Some code in parquet/tests/arrow_reader/io/mod.rs is unused. As I lack
  knowledge of the context, I just add just add #[allow(dead_code)].
@brunal brunal marked this pull request as ready for review February 9, 2026 14:12
ops: Arc<OperationLog>,
/// The (pre-parsed) parquet metadata for this file
// TODO: this is unused; consider removing it.
#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe for this dead code we can use it in a test somewhere to assert something useful? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I lack context to be the one to tackle it!

Copy link
Contributor

Choose a reason for hiding this comment

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

we can probably replace it with cfg(feature(async)) or something

Copy link
Contributor

Choose a reason for hiding this comment

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

I took the liberty of fixing this locally and pushing to this branch

@alamb
Copy link
Contributor

alamb commented Feb 11, 2026

Thank you @brunal 🙏

brunal and others added 2 commits February 11, 2026 21:12
Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
@brunal
Copy link
Contributor Author

brunal commented Feb 11, 2026

Comments addressed. I'm not sure whether this repo squashes commits when merging, or leaves them as-is. LMK if it's the latter than I'll force push a single nice commit.

@alamb
Copy link
Contributor

alamb commented Feb 11, 2026

Comments addressed. I'm not sure whether this repo squashes commits when merging, or leaves them as-is. LMK if it's the latter than I'll force push a single nice commit.

it squashes commits

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.

Look like a nice improvement to me -- thank you @brunal

I'll wait to see if @Jefffrey has any additional comments before merging

ops: Arc<OperationLog>,
/// The (pre-parsed) parquet metadata for this file
// TODO: this is unused; consider removing it.
#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

we can probably replace it with cfg(feature(async)) or something

@Jefffrey Jefffrey merged commit 7fbbde2 into apache:main Feb 13, 2026
17 checks passed
@Jefffrey
Copy link
Contributor

Thanks @brunal & @alamb

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

Labels

parquet Changes to the parquet crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants