Skip to content

refactor: Improve SessionContext::parse_duration API#20816

Merged
kosiew merged 3 commits intoapache:mainfrom
erenavsarogullari:improve_parse_duration_api
Mar 14, 2026
Merged

refactor: Improve SessionContext::parse_duration API#20816
kosiew merged 3 commits intoapache:mainfrom
erenavsarogullari:improve_parse_duration_api

Conversation

@erenavsarogullari
Copy link
Copy Markdown
Member

@erenavsarogullari erenavsarogullari commented Mar 9, 2026 •
edited
Loading

Which issue does this PR close?

Rationale for this change

This is follow-up PR for #20371.

Similarly to SessionContext::parse_capacity_limit API, SessionContext::parse_duration API also needs to have following improvements:

  1. Validation for empty or blank duration values:
SET datafusion.runtime.list_files_cache_ttl = ' '

Current:
DataFusion error: Error during planning: Failed to parse number from duration ' '

New:
DataFusion error: Error during planning: Duration should not be empty or blank for 'datafusion.runtime.list_files_cache_ttl'
  1. Exposing config name in error messages,
  2. Comprehensive test coverage for invalid durations,
  3. Updating datafusion.runtime.list_files_cache_ttl documentation for other allowed settings.

What changes are included in this PR?

Explained in the first section.

Are these changes tested?

Yes, being extended existing test cases.

Are there any user-facing changes?

Yes, config name has been added to error message when the validation is failed.

@github-actions github-actions bot added documentation Improvements or additions to documentation core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Mar 9, 2026
@erenavsarogullari erenavsarogullari force-pushed the improve_parse_duration_api branch 4 times, most recently from 230014c to 15c302a Compare March 10, 2026 02:47
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Mar 10, 2026
@erenavsarogullari
Copy link
Copy Markdown
Member Author

erenavsarogullari commented Mar 10, 2026

Hi @martin-g and @kosiew,
This is follow-up PR for #20371 and ready for the review when you have chance. Thanks in advance.

kosiew
kosiew requested changes Mar 10, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@kosiew kosiew left a comment

Choose a reason for hiding this comment

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

@erenavsarogullari

Thanks for working on this.

docs/source/library-user-guide/upgrading/52.0.0.md:54

The user-facing docs are still inconsistent for datafusion.runtime.list_files_cache_ttl. This upgrade guide continues to say the setting limits TTL “in seconds”, even though the parser accepts both minutes and seconds.

datafusion/core/src/execution/context/mod.rs Outdated
Comment on lines 1351 to 1352
@@ -1347,7 +1353,9 @@ impl SessionContext {
Copy link
Copy Markdown
Contributor

@kosiew kosiew Mar 10, 2026

Choose a reason for hiding this comment

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

parse_duration is doing unchecked u64 arithmetic when combining minutes and seconds:

For large inputs such as SET datafusion.runtime.list_files_cache_ttl = '307445734561825861m', minutes * 60 overflows before Duration::from_secs is reached. In debug/test builds that panics instead of returning a planning error.

This means malformed user input can still crash the statement path rather than producing a DataFusion error.
This needs an overflow-checked path (checked_mul / checked_add) and a test that exercises the oversized value through SET.

Extract a small helper for the checked accumulation here so the parser logic stays readable and the overflow error can consistently include config_name.

Copy link
Copy Markdown
Member Author

@erenavsarogullari erenavsarogullari Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

@kosiew Good point and addressed by the last commit. Thanks.

Copy link
Copy Markdown
Contributor

@kosiew kosiew Mar 13, 2026

Choose a reason for hiding this comment

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

@erenavsarogullari

a test that exercises the oversized value through SET.

I don't see this test.
Am I missing something?

Copy link
Copy Markdown
Member Author

@erenavsarogullari erenavsarogullari Mar 13, 2026

Choose a reason for hiding this comment

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

I have added test_parse_duration_with_overflow_check using SessionContext::parse_duration() for comprehensive coverage and SET Command is another use case to execute SessionContext::parse_duration() so i have also added SqlLogicalTests. Please check the last commit.

martin-g
martin-g reviewed Mar 10, 2026
View reviewed changes
@erenavsarogullari erenavsarogullari force-pushed the improve_parse_duration_api branch from dee482e to 821274a Compare March 11, 2026 04:31
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Mar 11, 2026
@erenavsarogullari erenavsarogullari force-pushed the improve_parse_duration_api branch from 0b58c3b to 77dea07 Compare March 11, 2026 04:37
@erenavsarogullari erenavsarogullari force-pushed the improve_parse_duration_api branch from 77dea07 to f2f7b59 Compare March 13, 2026 06:03
@erenavsarogullari erenavsarogullari force-pushed the improve_parse_duration_api branch from f2f7b59 to ad3518c Compare March 13, 2026 06:14
kosiew
kosiew approved these changes Mar 14, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@kosiew kosiew left a comment

Choose a reason for hiding this comment

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

Looks good to me now.

@kosiew kosiew added this pull request to the merge queue Mar 14, 2026
Merged via the queue into apache:main with commit 9c3c01a Mar 14, 2026
31 checks passed
@erenavsarogullari erenavsarogullari deleted the improve_parse_duration_api branch March 14, 2026 03:53
de-bgunter pushed a commit to de-bgunter/datafusion that referenced this pull request Mar 24, 2026
@erenavsarogullari @de-bgunter
refactor: Improve SessionContext::parse_duration API (apache#20816)
f183863 
## 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 apache#123` indicates that this PR will close issue apache#123.
-->

- Closes apache#20815.

## Rationale for this change
This is follow-up PR for
apache#20371.

Similarly to `SessionContext::parse_capacity_limit` API,
`SessionContext::parse_duration` API also needs to have following
improvements:

1. Validation for empty or blank duration values:
```
SET datafusion.runtime.list_files_cache_ttl = ' '

Current:
DataFusion error: Error during planning: Failed to parse number from duration ' '

New:
DataFusion error: Error during planning: Duration should not be empty or blank for 'datafusion.runtime.list_files_cache_ttl'

```
2. Exposing config name in error messages,
3. Comprehensive test coverage for invalid durations,
4. Updating `datafusion.runtime.list_files_cache_ttl` documentation for
other allowed settings.

## What changes are included in this PR?
Explained in the first section.

## Are these changes tested?
Yes, being extended existing test cases.

## Are there any user-facing changes?
Yes, config name has been added to error message when the validation is
failed.
This was referenced Apr 4, 2026
Closed
Merged
github-merge-queue bot pushed a commit that referenced this pull request Apr 7, 2026
@erenavsarogullari
test: Add datafusion.format.* configs test coverage (#21355)
7c3b22c 
## Which issue does this PR close?
- Closes #21354.

## Rationale for this change
Currently, DataFusion supports 9 `datafusion.format.*` configs but their
test coverage seem to be missed so this issue aims to add comprehensive
test coverage for them. This is follow-up to recent `config framework`
improvements: #20372 and
#20816.

## What changes are included in this PR?
New test coverage is being added for `datafusion.format.*` configs.

## Are these changes tested?
Yes, new test coverage is being added for `datafusion.format.*` configs.

## Are there any user-facing changes?
No
Dandandan pushed a commit to Dandandan/arrow-datafusion that referenced this pull request Apr 8, 2026
@erenavsarogullari @Dandandan
test: Add datafusion.format.* configs test coverage (apache#21355)
fe9c80c 
## Which issue does this PR close?
- Closes apache#21354.

## Rationale for this change
Currently, DataFusion supports 9 `datafusion.format.*` configs but their
test coverage seem to be missed so this issue aims to add comprehensive
test coverage for them. This is follow-up to recent `config framework`
improvements: apache#20372 and
apache#20816.

## What changes are included in this PR?
New test coverage is being added for `datafusion.format.*` configs.

## Are these changes tested?
Yes, new test coverage is being added for `datafusion.format.*` configs.

## Are there any user-facing changes?
No
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martin-g martin-g martin-g approved these changes

@kosiew kosiew kosiew approved these changes

Assignees

No one assigned

Labels

core Core DataFusion crate documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve SessionContext::parse_duration API

3 participants

@erenavsarogullari @martin-g @kosiew