Skip to content

Conversation

@AlyAbdelmoneim
Copy link

@AlyAbdelmoneim AlyAbdelmoneim commented Dec 27, 2025

Which issue does this PR close?

Rationale for this change

Currently, invalid writer_version values (e.g., "3.0") are accepted at SET time and only fail later when writing parquet files. This PR adds early validation so invalid values are rejected immediately with clear error messages, following the same pattern as ExplainFormat.

What changes are included in this PR?

  • Add DFWriterVersion enum with FromStr, Display, ConfigField implementations
  • Change ParquetOptions.writer_version from String to DFWriterVersion
  • Remove parse_version_string function (validation now happens at config time)
  • Update proto conversions to validate during deserialization
  • Add test for early validation

Are these changes tested?

Yes. Added test_parquet_writer_version_validation that verifies:

  • Valid values ("1.0", "2.0") are accepted
  • Invalid values ("3.0", "invalid") are rejected immediately at SET time
  • Error messages are clear and helpful

Are there any user-facing changes?

Not exactly. Invalid writer_version values now error immediately when set via SET command or proto deserialization, instead of failing later during parquet file writing. This provides better error messages and earlier feedback.
So the change is in the feedback not in the input.

Add ParquetWriterVersion enum to validate writer_version values
("1.0" or "2.0") immediately when setting via SET command or
proto deserialization, instead of failing later during parquet
file writing.

- Create ParquetWriterVersion enum with FromStr, Display, ConfigField
- Change ParquetOptions.writer_version from String to ParquetWriterVersion
- Remove parse_version_string function (validation now happens earlier)
- Update proto conversions to validate during deserialization
- Add test for early validation

This ensures invalid values are caught immediately with clear
error messages, following the same pattern as ExplainFormat.
Align naming with planned convention for validation enums.
@github-actions github-actions bot added common Related to common crate proto Related to proto crate labels Dec 27, 2025
@AlyAbdelmoneim
Copy link
Author

Hey @Jefffrey, thanks for your patience with my frequent mentions on this good-first-issue task, it's my first contribution here so I'm trying to make sure I'm on the right path! 😅

This PR validates the writer_version field in ParquetOptions. I added a new file parquet_config.rs that contains the DFWriterVersion enum and will house future validation enums for other ParquetOptions fields.

Let me know if you have any feedback!

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

Labels

common Related to common crate proto Related to proto crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant