chore: Move some utility methods to submodules of scalar_funcs #590
Conversation
|
@andygrove, @tshauck and @comphead would you mind to take you at this once CI passes? |
| } | ||
| } | ||
|
|
||
| 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.
|
Gently ping @comphead @andygrove and @kazuyukitanimura |
|
Merged. Thanks @advancedxy @kazuyukitanimura @tshauck @comphead |
|
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.