Skip to content

Conversation

@scovich
Copy link
Contributor

@scovich scovich commented Jun 20, 2025

Which issue does this PR close?

Housekeeping, part of

Rationale for this change

The variant module was starting to become unwieldy.

What changes are included in this PR?

Split out metadata, object, and list to sub-modules; move OffsetSize to the decoder module where it arguably belongs.

Result: variant.rs is "only" ~900 LoC instead of ~2kLoc.

Are there any user-facing changes?

No. Public re-exports should hide the change from users.

@github-actions github-actions bot added the parquet Changes to the parquet crate label Jun 20, 2025
@scovich
Copy link
Contributor Author

scovich commented Jun 20, 2025

NOTE: The code movement exposed some parts of the code that have no direct unit tests:

  • The utility methods in utils.rs (but heavily used by other modules)
  • The Variant struct itself in variant.rs (perhaps covered by the interop tests)

If we think the above represents an actual test gap, we may want to file some (good first) issues?

@alamb
Copy link
Contributor

alamb commented Jun 20, 2025

YES ! thank you @scovich

@alamb
Copy link
Contributor

alamb commented Jun 20, 2025

If we think the above represents an actual test gap, we may want to file some (good first) issues?

Sounds good to me

Note that doc examples I think function as unit tests as well, so we can include them too

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 @scovich

I'll fix the docs CI test and then merge this one in as it has a high potential to attract conflicts in other related PRs

@scovich
Copy link
Contributor Author

scovich commented Jun 20, 2025

I'll fix the docs CI test and then merge this one in as it has a high potential to attract conflicts in other related PRs

Whoop, I didn't see this. Just pushed a fix for the doc tests.

@scovich
Copy link
Contributor Author

scovich commented Jun 20, 2025

... or rather, just attempted to push; your fix won the race

@alamb
Copy link
Contributor

alamb commented Jun 20, 2025

Looks like I missed another one 🤦

@alamb alamb merged commit 7276819 into apache:main Jun 20, 2025
12 checks passed
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.

2 participants