You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "ByteBaker (via GitHub)" <gi...@apache.org> on 2026/07/03 21:39:15 UTC
[PR] fix: avoid panic parsing non-ASCII runtime config values [datafusion]
ByteBaker opened a new pull request, #23316:
URL: https://github.com/apache/datafusion/pull/23316
## Which issue does this PR close?
- Closes #22187
## Rationale for this change
Presence of a non-ascii character in the value while setting any datafusion runtime variable was panicking.
## What changes are included in this PR?
Better handling/iteration on character-boundary, and emits plan error now instead of panicking in case of such characters.
## Are these changes tested?
Yes. UTs have been updated to include such cases.
## Are there any user-facing changes?
No.
--
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] fix: avoid panic parsing non-ASCII runtime config values [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #23316:
URL: https://github.com/apache/datafusion/pull/23316#issuecomment-4882719458
merge CI queue removal issue should be fixed by #23314
--
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] fix: avoid panic parsing non-ASCII runtime config values [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #23316:
URL: https://github.com/apache/datafusion/pull/23316#issuecomment-4881517563
> Thanks @ByteBaker can we please explore if runtime config can be part of `SessionConfig`? In `SessionConfig` there is already a framework that enforces type checking.
I think one difference was that the RuntimeConfig is potentially shared across multiple SessionConfigs (and maybe can also be adjusted during plan execution)
I agree trying to consolidate RuntimeConfig and SessioNConfig should be considered as an independent project
--
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] fix: avoid panic parsing non-ASCII runtime config values [datafusion]
Posted by "ByteBaker (via GitHub)" <gi...@apache.org>.
ByteBaker commented on PR #23316:
URL: https://github.com/apache/datafusion/pull/23316#issuecomment-4880933622
@comphead yes that'd be a lot more cleaner/robust. However, that would also be a significant change in user-facing ergonomics.
My suggestion would be to keep this PR to ensure the panic is resolved, and separately start a discussion around doing that, since that's a larger change surface (and the question if we should remove all the runtime variables from the `SET xyz` semantics)
--
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] fix: avoid panic parsing non-ASCII runtime config values [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #23316:
URL: https://github.com/apache/datafusion/pull/23316#discussion_r3522950523
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1374,14 +1381,20 @@ impl SessionContext {
let mut seconds = None;
for duration in duration.split_inclusive(&['m', 's']) {
- let (number, unit) = duration.split_at(duration.len() - 1);
+ let (unit_start, unit) =
+ duration.char_indices().next_back().ok_or_else(|| {
Review Comment:
does this check have a test?
--
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] fix: avoid panic parsing non-ASCII runtime config values [datafusion]
Posted by "ByteBaker (via GitHub)" <gi...@apache.org>.
ByteBaker commented on PR #23316:
URL: https://github.com/apache/datafusion/pull/23316#issuecomment-4879487633
Hi @alamb for your review please.
--
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] fix: avoid panic parsing non-ASCII runtime config values [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead merged PR #23316:
URL: https://github.com/apache/datafusion/pull/23316
--
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] fix: avoid panic parsing non-ASCII runtime config values [datafusion]
Posted by "ByteBaker (via GitHub)" <gi...@apache.org>.
ByteBaker commented on code in PR #23316:
URL: https://github.com/apache/datafusion/pull/23316#discussion_r3523030676
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1374,14 +1381,20 @@ impl SessionContext {
let mut seconds = None;
for duration in duration.split_inclusive(&['m', 's']) {
- let (number, unit) = duration.split_at(duration.len() - 1);
+ let (unit_start, unit) =
+ duration.char_indices().next_back().ok_or_else(|| {
Review Comment:
Yes. The tests for empty value and non-ASCII value are covered by `test_parse_duration`. The `ok_or_else()` branch itself is defensive since `next_back()` can be `None` only for an empty string, which is rejected by the earlier `duration.trim().is_empty()` guard before the loop.
--
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