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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
