[FEAT] Add rpad and lpad expressions

[murex971]

Resolves #1931
Resolves #1930

[kevinzwang]

Good work so far! Biggest items of feedback are just in properly working with string characters instead of bytes.

[murex971]

@kevinzwang addressed the feedback.
Not sure why the tests are filing though, builds and tests running fine on my local.

[kevinzwang]

Not sure why the tests are filing though, builds and tests running fine on my local.

@murex971 the tests are failing because the function is_valid_input_lengths was removed from utf8.rs in the main branch. Could you try merging main into your branch and then resolving the errors that arise from the merged code?

Also pinging @jaychia because he made that change

[kevinzwang]

I believe the functionality of is_valid_input_lengths was inlined into parse_inputs, you could look into using that function in the kernel instead!

[jaychia]

I believe the functionality of is_valid_input_lengths was inlined into parse_inputs, you could look into using that function in the kernel instead!

Yes! We made it cleaner and easier to have parse_inputs check both the validity of the input lengths, and also return information about the expected output as a tuple: (is_full_null: bool, expected_length: usize)

Feel free to check out the other kernels for usage!

[murex971]

Yes! We made it cleaner and easier to have parse_inputs check both the validity of the input lengths, and also return information about the expected output as a tuple: (is_full_null: bool, expected_length: usize)

@jaychia
The implemented parse_inputs function takes in an array of input arrays (other_arrs), assuming they all would have same data types.

fn parse_inputs<T>(
    self_arr: &Utf8Array,
    other_arrs: &[&DataArray<T>],
) 

In the case of rpad one input is string and another is of numeric type, so the function cannot be directly use here, however I put the same logic inside the rpad function.
I am thinking of refactoring parse_inputs functionality by allowing it to take 2 different inputs of any type, this would incorporate all the case (would handle it in a different PR). Thoughts ?

cc: @kevinzwang

[kevinzwang]

Apologies for the delay. Happy to see these improvements to the code, and its awesome that you were also able to add lpad!

Overall, the code looks good to go, just a few small comments and suggestions. Make sure to also take a look at some of the unresolved comments from the previous review. Great work!

[kevinzwang]

I'll go ahead and approve it, should be ready to merge once these final things are resolved

[kevinzwang]

Looks good to go! Just one comment about the refactored pad function

[kevinzwang]

Small nit: I would recommend trying to do this match outside of the iteration so that we don't have an extra branching statement in the hot path

[murex971]

Updated.

[kevinzwang]

Amazing, I'll merge it in!