Improve performance of find_in_set function
#14020
Conversation
Signed-off-by: Tai Le Manh <[email protected]>
| .split(',') | ||
| .position(|s| s == string) | ||
| .map_or(0, |idx| idx + 1); | ||
| builder.append_value(T::Native::from_usize(position).unwrap()); |
There was a problem hiding this comment.
Probably faster if create Vec first and use PrimitiveArray::from()
There was a problem hiding this comment.
@jayzhan211 Thanks for reviewing,
Probably faster if create Vec first and use PrimitiveArray::from()
I have tried implementing it this way, it seems to have similar performance to this PR.
Here is the code I have tested:
pub fn find_in_set_general<'a, T, V>(
string_array: V,
str_list_array: V,
) -> Result<ArrayRef>
where
T: ArrowPrimitiveType,
T::Native: OffsetSizeTrait,
V: StringArrayType<'a, Item = &'a str>,
PrimitiveArray<T>: From<Vec<Option<<T as ArrowPrimitiveType>::Native>>>,
{
let string_iter = ArrayIter::new(string_array);
let str_list_iter = ArrayIter::new(str_list_array);
let mut result: Vec<Option<T::Native>> = Vec::with_capacity(string_iter.len());
string_iter
.zip(str_list_iter)
.for_each(
|(string_opt, str_list_opt)| match (string_opt, str_list_opt) {
(Some(string), Some(str_list)) => {
let position = str_list
.split(',')
.position(|s| s == string)
.map_or(0, |idx| idx + 1);
let value = T::Native::from_usize(position).unwrap();
result.push(Some(value));
}
_ => result.push(None),
},
);
let array = PrimitiveArray::<T>::from(result);
Ok(Arc::new(array) as ArrayRef)
}
There was a problem hiding this comment.
I think it is a good PR the way it is. One thing comes to my mind which probably relevant for other string functions as well.
So we do split(',') for every search pattern value, but most likely that argument will be literal like SELECT FIND_IN_SET(a, "s,q,l"); so we can do split only once.
I think we need to have a check a literal or column somewhere and we can optimize even further having this info, WDYT @tlm365 ?
|
@comphead Thanks for reviewing,
Oh yes, your suggestion is very reasonable. I will try to implement it. TYSM ❤ |
Signed-off-by: Tai Le Manh <[email protected]>
|
I just pushed some updates to support scalar args. Could you please take a look? @jayzhan211 @comphead |
|
Oh with literal support I think the code becomes much more tricky. |
@comphead I have added benchmark for case find_in_set_scalar/string_len_8
time: [57.247 µs 57.355 µs 57.477 µs]
change: [-77.622% -77.503% -77.415%] (p = 0.00 < 0.05)
Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
7 (7.00%) high mild
6 (6.00%) high severe
find_in_set_scalar/string_len_32
time: [57.872 µs 58.447 µs 59.215 µs]
change: [-77.737% -77.460% -77.182%] (p = 0.00 < 0.05)
Performance has improved.
Found 16 outliers among 100 measurements (16.00%)
16 (16.00%) high severe
find_in_set_scalar/string_len_1024
time: [58.024 µs 58.462 µs 58.993 µs]
change: [-77.049% -76.867% -76.660%] (p = 0.00 < 0.05)
Performance has improved.
Found 18 outliers among 100 measurements (18.00%)
3 (3.00%) high mild
15 (15.00%) high severeNOTE: |
Is possible to provide a test case so we can file a ticket if this is undesired behavior? The performance is definitely better, thanks @tlm365 I'm going to merge the PR |
| ], | ||
| Int32Array::from(vec![0, 0, 1, 2, 3]) | ||
| ); | ||
| test_find_in_set!( |
There was a problem hiding this comment.
Is possible to provide a test case so we can file a ticket if this is undesired behavior?
@comphead This is the test case I mentioned before. (test_find_in_set_with_scalar_args_2/3/4)
|
Looks like this PR is ready to go so I'll merge it in. Let's handle any follow on work with subsequent PRs. Thanks @tlm365 @comphead and @jayzhan211 |
Which issue does this PR close?
Closes #.
Rationale for this change
Improve performance of
find_in_setfunctionWhat changes are included in this PR?
Are these changes tested?
Yes. Added unit tests and benchmarks.
Are there any user-facing changes?
No.
*BENCHMARK RESULT