Uh oh!
There was an error while loading. Please reload this page.
chore: Move some utility methods to submodules of scalar_funcs - #590
Conversation
advancedxy
commented
Jun 20, 2024
@andygrove, @tshauck and @comphead would you mind to take you at this once CI passes? |
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| } | ||
| pub fn spark_xxhash64(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> { |
There was a problem hiding this comment.
Its not this PR problem but we need a description to pub methods
There was a problem hiding this comment.
Additionally, might consider limiting the scope. This function probably isn't needed outside of scalar_funcs.
There was a problem hiding this comment.
but we need a description to pub methods
I can add a description here.
Additionally, might consider limiting the scope. This function probably isn't needed outside of scalar_funcs.
Yeah, I originally limited it to pub(super). However we are accessing it in the benchmark module which needs public interface. I think we can leave it as it is and address access scope later by rewriting the benchmark code.
| match seed { | ||
| ColumnarValue::Scalar(ScalarValue::Int32(Some(seed))) => { | ||
| // iterate over the arguments to find out the length of the array | ||
| let num_rows = args[0..args.len() - 1] |
There was a problem hiding this comment.
any chance here to be an index out of bounds?
There was a problem hiding this comment.
I don't think so. The seed is always provided in the Spark/JVM side.
| } | ||
| } | ||
| pub fn spark_xxhash64(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> { |
There was a problem hiding this comment.
Additionally, might consider limiting the scope. This function probably isn't needed outside of scalar_funcs.
Uh oh!
There was an error while loading. Please reload this page.
kazuyukitanimura
left a comment
There was a problem hiding this comment.
Looks good. One minor question
Uh oh!
There was an error while loading. Please reload this page.
advancedxy
commented
Jun 24, 2024
Gently ping @comphead@andygrove and @kazuyukitanimura |
viirya
commented
Jun 25, 2024
Merged. Thanks @advancedxy@kazuyukitanimura@tshauck@comphead |
advancedxy
commented
Jun 25, 2024
Thanks everyone for reviewing. |
…e#590) * chore: Move some utility methods to submodules of scalar_funcs * Address comments
Which issue does this PR close?
Rationale for this change
This is a follow-up as discussed in #449 (comment)
What changes are included in this PR?
How are these changes tested?
Existing tests with one slightly modification.