From 5b23aee794866b25950e6c1d047c1bade64bb23a Mon Sep 17 00:00:00 2001 From: nuno-faria Date: Sat, 1 Aug 2026 17:28:01 +0100 Subject: [PATCH 1/3] fix: Correctly process numeric literals with underscores --- datafusion/sql/src/expr/mod.rs | 28 +++++++++++++++++++++ datafusion/sql/src/expr/value.rs | 18 ++++++++++--- datafusion/sqllogictest/test_files/expr.slt | 15 +++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/datafusion/sql/src/expr/mod.rs b/datafusion/sql/src/expr/mod.rs index c2e4822f76b99..b1de4e95fd8a2 100644 --- a/datafusion/sql/src/expr/mod.rs +++ b/datafusion/sql/src/expr/mod.rs @@ -1593,4 +1593,32 @@ mod tests { assert!(matches!(expr, Expr::Alias(_))); } + + #[test] + fn test_parse_numbers_with_underscores() { + use datafusion_common::ScalarValue::*; + + let context_provider = TestContextProvider::new(); + let sql_to_rel = SqlToRel::new(&context_provider); + + // (input, positive result, negative result) + let test_cases = [ + ("1_000", Int64(Some(1000)), Int64(Some(-1000))), + ("100_000", Int64(Some(100000)), Int64(Some(-100000))), + ("1_2_3_4", Int64(Some(1234)), Int64(Some(-1234))), + ("0_0", Int64(Some(0)), Int64(Some(-0))), + ("1_23.4_56", Float64(Some(123.456)), Float64(Some(-123.456))), + ]; + + for (literal, out_positive, out_negative) in test_cases { + assert_eq!( + sql_to_rel.parse_sql_number(literal, false).unwrap(), + Expr::Literal(out_positive, None) + ); + assert_eq!( + sql_to_rel.parse_sql_number(literal, true).unwrap(), + Expr::Literal(out_negative, None) + ); + } + } } diff --git a/datafusion/sql/src/expr/value.rs b/datafusion/sql/src/expr/value.rs index 13a47f545cf7e..d16c9b06dea87 100644 --- a/datafusion/sql/src/expr/value.rs +++ b/datafusion/sql/src/expr/value.rs @@ -74,10 +74,22 @@ impl SqlToRel<'_, S> { unsigned_number: &str, negative: bool, ) -> Result { - let signed_number: Cow = 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() }; // Try to parse as i64 first, then u64 if negative is false, then decimal or f64 diff --git a/datafusion/sqllogictest/test_files/expr.slt b/datafusion/sqllogictest/test_files/expr.slt index ba4e4d03b3c2d..32113890aadc0 100644 --- a/datafusion/sqllogictest/test_files/expr.slt +++ b/datafusion/sqllogictest/test_files/expr.slt @@ -2665,3 +2665,18 @@ false statement ok drop table t; + + +# Test numeric literals with underscore separators +# (https://github.com/apache/datafusion/issues/23877) + +statement ok +set datafusion.sql_parser.dialect = 'postgres' + +query IIIRI +select 1_000, 1_2_3_4, -1_2_3_4, 1_2.3_4, 0_0 +---- +1000 1234 -1234 12.34 0 + +statement ok +reset datafusion.sql_parser.dialect From e95b85b4304434549ccaf3ead053331be606e2b4 Mon Sep 17 00:00:00 2001 From: nuno-faria Date: Sat, 1 Aug 2026 17:49:42 +0100 Subject: [PATCH 2/3] Fix clippy --- datafusion/sql/src/expr/value.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/datafusion/sql/src/expr/value.rs b/datafusion/sql/src/expr/value.rs index d16c9b06dea87..9b96219e8b253 100644 --- a/datafusion/sql/src/expr/value.rs +++ b/datafusion/sql/src/expr/value.rs @@ -36,7 +36,6 @@ use sqlparser::ast::{ BinaryOperator, Expr as SQLExpr, Interval, UnaryOperator, Value, ValueWithSpan, }; use sqlparser::parser::ParserError::ParserError; -use std::borrow::Cow; use std::cmp::Ordering; use std::ops::Neg; use std::str::FromStr; From 5b40b5eafd2553e6f29e3c6e32175295d0e0adc6 Mon Sep 17 00:00:00 2001 From: nuno-faria Date: Tue, 4 Aug 2026 19:40:51 +0100 Subject: [PATCH 3/3] Avoid extra allocation with positive numbers without underscores --- datafusion/sql/src/expr/value.rs | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/datafusion/sql/src/expr/value.rs b/datafusion/sql/src/expr/value.rs index 9b96219e8b253..1307e917e4251 100644 --- a/datafusion/sql/src/expr/value.rs +++ b/datafusion/sql/src/expr/value.rs @@ -36,6 +36,7 @@ use sqlparser::ast::{ BinaryOperator, Expr as SQLExpr, Interval, UnaryOperator, Value, ValueWithSpan, }; use sqlparser::parser::ParserError::ParserError; +use std::borrow::Cow; use std::cmp::Ordering; use std::ops::Neg; use std::str::FromStr; @@ -73,22 +74,27 @@ impl SqlToRel<'_, S> { unsigned_number: &str, negative: bool, ) -> Result { - 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 signed_number = if !negative && !unsigned_number.contains('_') { + Cow::Borrowed(unsigned_number) + } else { + let mut signed_number = + String::with_capacity(unsigned_number.len() + usize::from(negative)); + if negative { + signed_number.push('-'); } - }); + unsigned_number.bytes().for_each(|b| { + if b != b'_' { + signed_number.push(b as char); + } + }); + Cow::Owned(signed_number) + }; let unsigned_number = if negative { &signed_number[1..] } else { - signed_number.as_str() + &signed_number }; // Try to parse as i64 first, then u64 if negative is false, then decimal or f64