fix: Correctly process numeric literals with underscores

[nuno-faria]

Which issue does this PR close?

• Closes Numeric SQL literals with underscores are not correctly parsed #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.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.89%. Comparing base (179b32c) to head (df66225).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #24046   +/-   ##
=======================================
  Coverage   80.89%   80.89%           
=======================================
  Files        1102     1102           
  Lines      376196   376227   +31     
  Branches   376196   376227   +31     
=======================================
+ Hits       304326   304354   +28     
+ Misses      53759    53758    -1     
- Partials    18111    18115    +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Jefffrey]

i think we need to account for edge cases like multiple underscores in a row, and trailing underscores

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

[getChan]

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 != '_'); 

[nuno-faria]

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):

Here is the code I used to test:

main.rs

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");
}

Without negative numbers they are similar in performance.

[nuno-faria]

i think we need to account for edge cases like multiple underscores in a row, and trailing underscores

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):

> select 10__00;                                                                                     
+------+
| __00 |
+------+
| 10   |
+------+

[Jefffrey]

• raised apache/datafusion-sqlparser-rs#2421 upstream

[kumarUjjawal]

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.

[kumarUjjawal]

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.

[nuno-faria]

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.

[nuno-faria]

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.