You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "etolbakov (via GitHub)" <gi...@apache.org> on 2025/09/21 13:43:03 UTC
[PR] feat: Ensure config setttings are set to valid values [datafusion]
etolbakov opened a new pull request, #17697:
URL: https://github.com/apache/datafusion/pull/17697
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes # https://github.com/apache/datafusion/issues/17498
## Rationale for this change
Improved UX. The user would be given hints when attempts to set wrong/unsupported value.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
TBD
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
- adjusted existing unit tests
- additional sql logic tests
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
yes
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Ensure config setttings are set to valid values [datafusion]
Posted by "etolbakov (via GitHub)" <gi...@apache.org>.
etolbakov commented on PR #17697:
URL: https://github.com/apache/datafusion/pull/17697#issuecomment-3319470405
Thank you so much for your inputs @Jefffrey !
I'll proceed with the plan and ping you once the PR is in more or less ready state if you don't mind,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Ensure config setttings are set to valid values [datafusion]
Posted by "etolbakov (via GitHub)" <gi...@apache.org>.
etolbakov commented on PR #17697:
URL: https://github.com/apache/datafusion/pull/17697#issuecomment-3317436427
Upon reviewing the requirements and the example for `ExplainFormat` I identified several distinct cases:
1. **Parameters with Numeric/Boolean Types or Free-Text Strings:**
No changes are needed for these parameters.
2. **Parameters with Existing Validation:**
The main consideration here is whether to standardize error formatting for consistency. I’ve left a few TODOs to revisit this.
3. **Parameters Similar to "ExplainFormat":**
These strings can be converted to enums, following a clear approach. I’ve already applied a similar change to "NullOrdering".
Determining the best location for enum declarations within the config can be challenging. Unless we establish a rule-of-thumb (e.g., placing each enum in a separate file), I’ll experiment further and propose a solution.
For existing enums (like "NullOrdering"), reusing them should be the priority. However, I’m unsure if it’s beneficial to move specific logic (such as the "nulls_first" method) closer to the config.
4. **Parameters Similar to "DurationFormat":**
These involve strings in the Datafusion config that map to enums from other crates (e.g., "datafusion.format.duration_format", "datafusion.sql_parser.dialect").
Due to the orphan rule, I couldn’t implement the "Display" or "FromStr" traits directly. My current solution is to create wrapper types (named as DF<OriginalType>, such as "DFDurationFormat"). I’m open to suggestions for a better naming convention.
@Jefffrey could you please have a look at the plan above and tell me what do you think?
Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Ensure config setttings are set to valid values [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #17697: feat: Ensure config setttings are set to valid values
URL: https://github.com/apache/datafusion/pull/17697
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Ensure config setttings are set to valid values [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #17697:
URL: https://github.com/apache/datafusion/pull/17697#issuecomment-3319338919
> Upon reviewing the requirements and the example for `ExplainFormat` I identified several distinct cases:
>
> 1. **Parameters with Numeric/Boolean Types/Date formats or Free-Text Strings:**
> No changes are needed for these parameters.
>
> 2. **Parameters with Existing Validation:**
> The main consideration here is whether to standardize error formatting for consistency. I’ve left a few TODOs to revisit this.
>
> 3. **Parameters Similar to "ExplainFormat":**
> These strings can be converted to enums, following a clear approach. I’ve already applied a similar change to "NullOrdering".
> Determining the best location for enum declarations within the config can be challenging. Unless we establish a rule-of-thumb (e.g., placing each enum in a separate file), I’ll experiment further and propose a solution.
> For existing enums (like "NullOrdering"), reusing them should be the priority. However, I’m unsure if it’s beneficial to move specific logic (such as the "nulls_first" method) closer to the config.
>
> 4. **Parameters Similar to "DurationFormat":**
> These involve strings in the Datafusion config that map to enums from other crates (e.g., "datafusion.format.duration_format", "datafusion.sql_parser.dialect").
> Due to the orphan rule, I couldn’t implement the "Display" or "FromStr" traits directly. My current solution is to create wrapper types (named as DF, such as "DFDurationFormat"). I’m open to suggestions for a better naming convention.
>
>
> @Jefffrey could you please have a look at the plan above and tell me what do you think? Thank you!
For point 1 I wouldn't be so quick to dismiss numeric fields; we might need to put some checks on them (e.g. nonzero) for sanity. Although we could look into changing the type to [`NonZeroUsize`](https://doc.rust-lang.org/std/num/type.NonZeroUsize.html) for some of them and maybe that would enforce it automatically for us 🤔
Point 2 sounds good, but yeah it might need a follow-up ticket as I assume it might be quite a bit of work itself.
Point 3 I don't think we want to go too extreme and have each enum in it's own file. For moving existing, perhaps it's better to leave them where they are for the moment 🤔
Point 4 I've only skimmed the code a bit so far, but that makes sense to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Ensure config setttings are set to valid values [datafusion]
Posted by "etolbakov (via GitHub)" <gi...@apache.org>.
etolbakov commented on PR #17697:
URL: https://github.com/apache/datafusion/pull/17697#issuecomment-3367024523
A status update on the ticket.
The following settings have enum instead of a string (with DatafusionError accordingly )
```shell
| 1 | datafusion.execution.parquet.compression ✅ | zstd(3) |
| 1.1 | datafusion.execution.parquet.statistics_enabled ✅ | page |
| 1.2 | datafusion.execution.parquet.encoding ✅ | NULL |
| 2 | datafusion.execution.spill_compression ✅(already enum) | uncompressed |
| 3 | datafusion.explain.format ✅(already enum) | indent |
| 4 | datafusion.sql_parser.dialect ✅ | generic |
| 5 | datafusion.sql_parser.default_null_ordering ✅ | nulls_max |
| 6 | datafusion.format.duration_format ✅ | pretty |
```
<img width="1255" height="401" alt="Screenshot 2025-10-03 at 20 45 33" src="https://github.com/user-attachments/assets/9a37ab98-4159-42cc-86ab-4032fb2eaf5e" />
Need to tidy up the PR, remove unnecessary code, fix failing tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Ensure config setttings are set to valid values [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #17697:
URL: https://github.com/apache/datafusion/pull/17697#issuecomment-3609678058
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org