Refactor signatures for lpad, rpad, left, and right

[jiashenC]

Which issue does this PR close?

Refactor signatures for lpad, rpad, left, and right. They share very similar signatures.

Closes some tasks in #13301.

What changes are included in this PR?

Signature changes.

Are these changes tested?

Existing tests.

Are there any user-facing changes?

No

[Omega359]

You can run

cargo test --test sqllogictests

locally to reproduce the test failures.

[jayzhan211]

I'm thinking maybe we should avoid casting type if they have the same Logical type like utf8 -> utf8view. 🤔

[Omega359]

I would agree. If the signature allows for a string and receives a Utf8 it should accept it as is unless it needs to be coerced to a common type for some other reason. The less casting the better imho

[alamb]

100% -- casts can often require trivial computation during query -- in this particular case casting letter2 to a Utf8View means it will copy at least an additional 128 bytes for each row (each view is 128 bytes)

[jiashenC]

Thanks for the suggestion. I implement not to cast if their logical types are the same. However, it is failing in some cases where Dictionary is in the signature. In those cases, the logical type is the same, but the native type is Dictionary causing type mismatch. The test can be reproduced

cargo test --test sqllogictests -- jctest

Any suggestions?

[jayzhan211]

We might need to implement the kernel for dictionary type.

[jiashenC]

I see. Can you elaborate on how we implement the kernel for dictionary type? I can give it a try.

[jayzhan211]

Check the fn invoke function for each function.

For example lpad

fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
        match args[0].data_type() {
            Utf8 | Utf8View => make_scalar_function(lpad::<i32>, vec![])(args),
            LargeUtf8 => make_scalar_function(lpad::<i64>, vec![])(args),
            other => exec_err!("Unsupported data type {other:?} for function lpad"),
        }
    }

It support utf8/utf8view/largeutf8, but not dictionary. You can rewrite it like this

fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
        invoke_inner(args, &args[0].data_type())
}

fn invoke_inner(args: &[ColumnarValue], data_type: &DataType) -> Result<ColumnarValue> {
    match data_type {
        DataType::Dictionary(_, v) => {
            invoke_inner(args, v.as_ref())
        }
        Utf8 | Utf8View => make_scalar_function(lpad::<i32>, vec![])(args),
        LargeUtf8 => make_scalar_function(lpad::<i64>, vec![])(args),
        other => exec_err!("Unsupported data type {other:?} for function lpad"),
    }
}

[jiashenC]

Thanks for the pointer! I added the support for left function. Can you give a read of whether that is a good design?

[jiashenC]

Hey @jayzhan211, I have added dictionary support for those four functions and repeat because the logical type casting skipping fails some relevant tests. Please let me know if you have any thoughts on the current implementation.

I also have a question when getting values from Dictionary to get the actual string, it drops NULL values, which causes some tests to fail. Is there any helper method I can use to get NULL preserving values from the Dictionary?