Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Feb 10, 2026

Which issue does this PR close?

Rationale for this change

While reviewing this PR, I found (with codex) some additional code paths that I think would be valuable to test:

  1. That you can't set Some(0) for the max sizes
  2. Certain codepaths

What changes are included in this PR?

Add tets

Are these changes tested?

Are there any user-facing changes?

@alamb alamb marked this pull request as draft February 10, 2026 21:53
@github-actions github-actions bot added the parquet Changes to the parquet crate label Feb 10, 2026
assert_eq!(total_rows, 100, "Total rows should be preserved");
}

#[test]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you run code coverage like this

 cargo llvm-cov --html -p parquet --features=arrow,async

You'll see there are a few missing lines in parquet/src/arrow/arrow_writer/mod.rs.html

Specifically:

Image

This test covers that branch

@alamb alamb changed the title Minor: Add additinal test coverage for WriterProperties::{max_row_group_row_count,max_row_group_size} Minor: Add additional test coverage for WriterProperties::{max_row_group_row_count,max_row_group_size} Feb 10, 2026
@alamb alamb marked this pull request as ready for review February 10, 2026 22:23
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.

LGTM. Thanks @alamb

Copy link
Contributor

@yonipeleg33 yonipeleg33 left a comment

Choose a reason for hiding this comment

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

Thanks, great stuff!

@alamb alamb merged commit 55a769f into apache:main Feb 11, 2026
18 checks passed
@alamb
Copy link
Contributor Author

alamb commented Feb 11, 2026

Thanks @etseidl and @yonipeleg33

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