-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: Ensure catalog format in config is valid #19498
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
base: main
Are you sure you want to change the base?
feat: Ensure catalog format in config is valid #19498
Conversation
- Add CatalogFormat enum with valid format values (CSV, PARQUET, JSON, NDJSON, AVRO, ARROW) - Implement FromStr for CatalogFormat with validation and clear error messages - Implement Display and ConfigField traits for CatalogFormat - Update CatalogOptions.format from Option<String> to Option<CatalogFormat> - Update session_state_defaults.rs to work with enum - Add test for catalog format validation
|
@Jefffrey Hey, I opened this PR to validate catalog format, since I didn't want to have all validations in one PR as you requested. I think it's important you review that my assumption for the default is correct. In
This is needed because if this PR is good I will proceede with the other validations. |
|
I don't know if this config field is one we should be tackling. I don't think we can ever have an exhaustive list of valid values here if we consider user extensibility? |
Oh you are right! I just wasn't aware of that use case where users can register custom table provider factories. I saw the For my next PR, I'll be working on validating Fields to validate with enum wrappers (following the pattern from PR #17549):
And I have a question for this field
if you think this are the edits we want, I will create a new PR for these changes. |
|
Thanks for the detailed reply; I can't really look at it all in detail right now, but for the compression question I'll just say we should keep things simple for now. If we run into a complex case we should defer that as one of the later changes, in case it requires larger refactors. I do recall there was some discussion in that vein recently: |
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
change
catalog.formatfromstringtoCatalogFormat.Are these changes tested?
Yes.
Are there any user-facing changes?
No.