You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "nuno-faria (via GitHub)" <gi...@apache.org> on 2026/08/01 16:33:53 UTC
[PR] fix: Correctly process numeric literals with underscores [datafusion]
nuno-faria opened a new pull request, #24046:
URL: https://github.com/apache/datafusion/pull/24046
## Which issue does this PR close?
- Closes #23877.
## Rationale for this change
Be able to use numeric literals with underscores as separators, which are valid in some dialects like Postgres.
## What changes are included in this PR?
The `parse_sql_number` method now removes underscores from numbers first before passing to the Rust parser.
## Are these changes tested?
Yes.
## 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: Correctly process numeric literals with underscores [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#discussion_r3717763363
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -74,10 +73,22 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
unsigned_number: &str,
negative: bool,
) -> Result<Expr> {
- let signed_number: Cow<str> = if negative {
- Cow::Owned(format!("-{unsigned_number}"))
+ let mut signed_number =
Review Comment:
Thanks, that makes sense.
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "nuno-faria (via GitHub)" <gi...@apache.org>.
nuno-faria commented on PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#issuecomment-5183243261
> Could we validate underscore placement before removing it? The SQL parser can pass values such as 1._2 and 1_e2 here. This change would turn them into valid 1.2 and 1e2, although PostgreSQL rejects them. We can reject underscores that do not have a digit on both sides.
Thanks @kumarUjjawal for the review. I think it's best to leave that to the upstream crate instead of tackling it here, since other systems using that library with the Postgres dialect will also have the same issue. My idea here of removing the separators is simply to allow the numbers to be parsed by the Rust library.
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #24046:
URL: https://github.com/apache/datafusion/pull/24046
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#issuecomment-5187284095
> > Could we validate underscore placement before removing it? The SQL parser can pass values such as 1._2 and 1_e2 here. This change would turn them into valid 1.2 and 1e2, although PostgreSQL rejects them. We can reject underscores that do not have a digit on both sides.
>
> Thanks @kumarUjjawal for the review. I think it's best to leave that to the upstream crate instead of tackling it here, since other systems using that library with the Postgres dialect will also have the same issue. My idea here of removing the separators is simply to allow the numbers to be parsed by the Rust library.
Thanks, that make sense.
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "nuno-faria (via GitHub)" <gi...@apache.org>.
nuno-faria commented on code in PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#discussion_r3715303968
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -74,10 +73,22 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
unsigned_number: &str,
negative: bool,
) -> Result<Expr> {
- let signed_number: Cow<str> = if negative {
- Cow::Owned(format!("-{unsigned_number}"))
+ let mut signed_number =
Review Comment:
Thanks, I changed it. It does require an extra iteration when the string has underscores (with `contains`), but I profiled the code and the cost is minimal vs the improvement on positive numbers without underscores.
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#discussion_r3717763363
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -74,10 +73,22 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
unsigned_number: &str,
negative: bool,
) -> Result<Expr> {
- let signed_number: Cow<str> = if negative {
- Cow::Owned(format!("-{unsigned_number}"))
+ let mut signed_number =
Review Comment:
Thanks.
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#discussion_r3713633812
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -74,10 +73,22 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
unsigned_number: &str,
negative: bool,
) -> Result<Expr> {
- let signed_number: Cow<str> = if negative {
- Cow::Owned(format!("-{unsigned_number}"))
+ let mut signed_number =
Review Comment:
could we avoid creating a new string when the number is positive and contains no underscores? The current change adds one allocation per number, which could matter for large VALUES queries.
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#issuecomment-5155058719
i think we need to account for edge cases like multiple underscores in a row, and trailing underscores
```sql
postgres=# select 12__10;
ERROR: trailing junk after numeric literal at or near "12__10"
LINE 1: select 12__10;
postgres=# select 12_10_;
ERROR: trailing junk after numeric literal at or near "12_10_"
LINE 1: select 12_10_;
```
i think this code allows that whilst postgres forbids it
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "getChan (via GitHub)" <gi...@apache.org>.
getChan commented on code in PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#discussion_r3698436248
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -74,10 +73,22 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
unsigned_number: &str,
negative: bool,
) -> Result<Expr> {
- let signed_number: Cow<str> = if negative {
- Cow::Owned(format!("-{unsigned_number}"))
+ let mut signed_number =
+ String::with_capacity(unsigned_number.len() + usize::from(negative));
+ if negative {
+ signed_number.push('-');
+ }
+ // remove underscores, since the Rust parser used here does not support them
+ unsigned_number.bytes().for_each(|b| {
+ if b != b'_' {
+ signed_number.push(b as char);
+ }
+ });
+
+ let unsigned_number = if negative {
+ &signed_number[1..]
} else {
- Cow::Borrowed(unsigned_number)
+ signed_number.as_str()
Review Comment:
nit : how about `retain()`? for cleanup
same memory and iteration
and
Since we always allocate a new string now instead of borrowing(`Cow::Borrowed`), handling negative seems unnecessary.
```
let mut clean = unsigned_number.to_string();
clean.retain(|c| c != '_');
let signed_number = if negative { format!("-{clean}") } else { clean.clone() };
```
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "getChan (via GitHub)" <gi...@apache.org>.
getChan commented on code in PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#discussion_r3698436248
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -74,10 +73,22 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
unsigned_number: &str,
negative: bool,
) -> Result<Expr> {
- let signed_number: Cow<str> = if negative {
- Cow::Owned(format!("-{unsigned_number}"))
+ let mut signed_number =
+ String::with_capacity(unsigned_number.len() + usize::from(negative));
+ if negative {
+ signed_number.push('-');
+ }
+ // remove underscores, since the Rust parser used here does not support them
+ unsigned_number.bytes().for_each(|b| {
+ if b != b'_' {
+ signed_number.push(b as char);
+ }
+ });
+
+ let unsigned_number = if negative {
+ &signed_number[1..]
} else {
- Cow::Borrowed(unsigned_number)
+ signed_number.as_str()
Review Comment:
nit : how about `retain()`? for cleanup
same memory and iteration
```
let mut signed_number = if negative {
format!("-{unsigned_number}")
} else {
unsigned_number.to_string()
};
signed_number.retain(|c| c != '_');
```
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#issuecomment-5152507397
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24046?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 80.86%. Comparing base ([`b902256`](https://app.codecov.io/gh/apache/datafusion/commit/b9022565e294072d7bca039f743b1cacdf10dee1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e95b85b`](https://app.codecov.io/gh/apache/datafusion/commit/e95b85b4304434549ccaf3ead053331be606e2b4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24046 +/- ##
==========================================
- Coverage 80.86% 80.86% -0.01%
==========================================
Files 1101 1101
Lines 375446 375474 +28
Branches 375446 375474 +28
==========================================
+ Hits 303598 303614 +16
- Misses 53758 53767 +9
- Partials 18090 18093 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24046?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "getChan (via GitHub)" <gi...@apache.org>.
getChan commented on code in PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#discussion_r3698436248
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -74,10 +73,22 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
unsigned_number: &str,
negative: bool,
) -> Result<Expr> {
- let signed_number: Cow<str> = if negative {
- Cow::Owned(format!("-{unsigned_number}"))
+ let mut signed_number =
+ String::with_capacity(unsigned_number.len() + usize::from(negative));
+ if negative {
+ signed_number.push('-');
+ }
+ // remove underscores, since the Rust parser used here does not support them
+ unsigned_number.bytes().for_each(|b| {
+ if b != b'_' {
+ signed_number.push(b as char);
+ }
+ });
+
+ let unsigned_number = if negative {
+ &signed_number[1..]
} else {
- Cow::Borrowed(unsigned_number)
+ signed_number.as_str()
Review Comment:
nit : how about `retain()`? for cleanup
```
let mut signed_number = if negative {
format!("-{unsigned_number}")
} else {
unsigned_number.to_string()
};
signed_number.retain(|c| c != '_');
```
same memory and iteration
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "nuno-faria (via GitHub)" <gi...@apache.org>.
nuno-faria commented on code in PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#discussion_r3698879330
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -74,10 +73,22 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
unsigned_number: &str,
negative: bool,
) -> Result<Expr> {
- let signed_number: Cow<str> = if negative {
- Cow::Owned(format!("-{unsigned_number}"))
+ let mut signed_number =
+ String::with_capacity(unsigned_number.len() + usize::from(negative));
+ if negative {
+ signed_number.push('-');
+ }
+ // remove underscores, since the Rust parser used here does not support them
+ unsigned_number.bytes().for_each(|b| {
+ if b != b'_' {
+ signed_number.push(b as char);
+ }
+ });
+
+ let unsigned_number = if negative {
+ &signed_number[1..]
} else {
- Cow::Borrowed(unsigned_number)
+ signed_number.as_str()
Review Comment:
Thanks @getChan, that version looks simpler but I think it ends up doing more work. Here is a profiling that compares both with negative numbers (`version_two` is the original):
<img width="2556" height="703" alt="image" src="https://github.com/user-attachments/assets/bd23a98d-d2d3-4514-a406-b513594eb175" />
Here is the code I used to test:
<details>
<summary>main.rs</summary>
```rust
use std::time::Instant;
const ITERS: usize = 100_000_000;
fn version_one(input: &str, negative: bool) {
let mut signed_number = if negative {
format!("-{input}")
} else {
input.to_string()
};
signed_number.retain(|c| c != '_');
let _unsigned_number = if negative {
&signed_number[1..]
} else {
signed_number.as_str()
};
}
fn version_two(input: &str, negative: bool) {
let mut signed_number = String::with_capacity(input.len() + usize::from(negative));
if negative {
signed_number.push('-');
}
for b in input.bytes() {
if b != b'_' {
signed_number.push(b as char);
}
}
let _unsigned_number = if negative {
&signed_number[1..]
} else {
signed_number.as_str()
};
}
fn main() {
let input = "123_456_789_012_345_678_901_234_567_890";
let negative = false;
let start = Instant::now();
for _ in 0..ITERS {
version_one(input, negative);
}
let v1 = start.elapsed().as_nanos() as f64 / ITERS as f64;
let start = Instant::now();
for _ in 0..ITERS {
version_two(input, negative);
}
let v2 = start.elapsed().as_nanos() as f64 / ITERS as f64;
println!("version 1: {v1:.2} ns/op");
println!("version 2: {v2:.2} ns/op");
}
```
</details>
Without negative numbers they are similar in performance.
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "nuno-faria (via GitHub)" <gi...@apache.org>.
nuno-faria commented on PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#issuecomment-5157376207
> i think we need to account for edge cases like multiple underscores in a row, and trailing underscores
>
> ```sql
> postgres=# select 12__10;
> ERROR: trailing junk after numeric literal at or near "12__10"
> LINE 1: select 12__10;
> postgres=# select 12_10_;
> ERROR: trailing junk after numeric literal at or near "12_10_"
> LINE 1: select 12_10_;
> ```
>
> i think this code allows that whilst postgres forbids it
Thanks @Jefffrey. I think that would need to be done directly in the `sqlparser` crate. As far as I'm aware, the numbers that reach the `parse_sql_number` are always valid. For example, with `10__00`, the number that reaches is `10`, while `__10` is treated as the alias (like DuckDB does):
```sql
> select 10__00;
+------+
| __00 |
+------+
| 10 |
+------+
```
--
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: Correctly process numeric literals with underscores [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24046:
URL: https://github.com/apache/datafusion/pull/24046#issuecomment-5225808877
Thanks everyone. Merging this in to get it for DF 54
--
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