Optimize gcd for array and scalar case by avoiding make_scalar_function where has unnecessary conversion between scalar and array
#14834
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
sqllogictest
|
alamb
approved these changes
Feb 25, 2025
datafusion/functions/src/math/gcd.rs
Outdated
Comment on lines
115
to
127
| fn compute_gcd_for_arrays(a: &ArrayRef, b: &ArrayRef) -> Result<ColumnarValue> { | ||
| let result: Result<Int64Array> = a | ||
| .as_primitive::<Int64Type>() | ||
| .iter() | ||
| .zip(b.as_primitive::<Int64Type>().iter()) | ||
| .map(|(a, b)| match (a, b) { | ||
| (Some(a), Some(b)) => Ok(Some(compute_gcd(a, b)?)), | ||
| _ => Ok(None), | ||
| }) | ||
| .collect(); | ||
|
|
||
| result.map(|arr| ColumnarValue::Array(Arc::new(arr) as ArrayRef)) | ||
| } |
There was a problem hiding this comment.
I think you can use try_binary and make it even faster:
Suggested change
| fn compute_gcd_for_arrays(a: &ArrayRef, b: &ArrayRef) -> Result<ColumnarValue> { | |
| let result: Result<Int64Array> = a | |
| .as_primitive::<Int64Type>() | |
| .iter() | |
| .zip(b.as_primitive::<Int64Type>().iter()) | |
| .map(|(a, b)| match (a, b) { | |
| (Some(a), Some(b)) => Ok(Some(compute_gcd(a, b)?)), | |
| _ => Ok(None), | |
| }) | |
| .collect(); | |
| result.map(|arr| ColumnarValue::Array(Arc::new(arr) as ArrayRef)) | |
| } | |
| fn compute_gcd_for_arrays(a: &ArrayRef, b: &ArrayRef) -> Result<ColumnarValue> { | |
| let result: Result<Int64Array> = a | |
| .as_primitive::<Int64Type>() | |
| .iter() | |
| .zip(b.as_primitive::<Int64Type>().iter()) | |
| .map(|(a, b)| match (a, b) { | |
| (Some(a), Some(b)) => Ok(Some(compute_gcd(a, b)?)), | |
| _ => Ok(None), | |
| }) | |
| .collect(); | |
| result.map(|arr| ColumnarValue::Array(Arc::new(arr) as ArrayRef)) | |
| } |
I'll run some quick numbers with your new benchmark.
There was a problem hiding this comment.
With my proposal the two array version seems to go 25% faster than this PR. Here is a PR with that improvement:
Gnuplot not found, using plotters backend
Benchmarking gcd both array: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.9s, enable flat sampling, or reduce sample count to 60.
gcd both array time: [1.1940 ms 1.2007 ms 1.2081 ms]
change: [-27.574% -27.080% -26.470%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
6 (6.00%) high mild
3 (3.00%) high severe
gcd array and scalar time: [1.9644 ms 1.9705 ms 1.9781 ms]
change: [-1.3293% -0.9077% -0.4670%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 6 outliers among 100 measurements (6.00%)
3 (3.00%) high mild
3 (3.00%) high severe
gcd both scalar time: [26.094 ns 26.253 ns 26.467 ns]
change: [-1.1099% -0.6987% -0.2362%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 8 outliers among 100 measurements (8.00%)
4 (4.00%) high mild
4 (4.00%) high severe
Use try_binary to make gcd even faster
jayzhan211
commented
Feb 25, 2025
| b.len() | ||
| ); | ||
| } | ||
| try_binary(a, b, compute_gcd) |
There was a problem hiding this comment.
try_binary actually did the length check
|
Thanks @alamb |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
make_scalar_functionconvert scalar to array and call function kernel. However, we don't need to convert to array and can directly compute on it. This improve the gcd performance in array vs scalar caseWhat changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?