Jefffrey commented on code in PR #23930:
URL: https://github.com/apache/datafusion/pull/23930#discussion_r3680304635
##########
datafusion/functions/src/unicode/character_length.rs:
##########
@@ -59,11 +62,16 @@ impl Default for CharacterLengthFunc {
impl CharacterLengthFunc {
pub fn new() -> Self {
- use DataType::*;
Self {
- signature: Signature::uniform(
- 1,
- vec![Utf8, LargeUtf8, Utf8View],
Review Comment:
at this point its more a question of if we should keep the existing
behaviour or not 🤔
it looks like we didnt rely on the previous behaviour on our test suite (?)
but maybe a downstream user could have used it. i am in favour of having a
stricter signature, but i know in general we've been limiting breaking changes
where possible
(i do agree things like reversing the array looks off)
if we substitute in `TypeSignatureClass::Any` for
`TypeSignatureClass::Native(logical_binary())` does it keep the previous
behaviour? then we could in a separate followup explicitly tighten the input
signature so at least its more visible in its own PR
##########
datafusion/functions/src/unicode/reverse.rs:
##########
@@ -83,7 +91,9 @@ impl ScalarUDFImpl for ReverseFunc {
fn invoke_with_args(&self, args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
let args = &args.args;
match args[0].data_type() {
- Utf8 | Utf8View | LargeUtf8 => make_scalar_function(reverse,
vec![])(args),
+ Utf8 | Utf8View | LargeUtf8 | DataType::Dictionary(_, _) => {
Review Comment:
we can probably just remove this datatype check here, its unnecessary since
signature guards it for us
--
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]
