feat(spark): Custom annotation for more string functions

[VaggelisD]

This is a follow up on #4004, as during my investigation for SUBSTRING I came across other Spark string functions (CONCAT, LPAD, RPAD) which are input type dependent.

Although it's not documented clearly, these functions will return a BINARY only if all their "string" operands are BINARY, otherwise they return a STRING:

spark-sql (default)> with tbl as (select cast('test' as binary) as bin_col, 'str' as str_col) 
select 
 typeof(concat(bin_col, bin_col)), 
 typeof(concat(str_col, bin_col)), 
 typeof(concat(bin_col, str_col)), 
 typeof(concat(str_col, str_col)) 
from tbl;

binary  string  string  string

spark-sql (default)> with tbl as (select cast('test' as binary) as bin_col, 'str' as str_col) 
select 
 typeof(lpad(bin_col, 1, bin_col)), 
 typeof(lpad(str_col, 1, bin_col)), 
 typeof(lpad(bin_col, 1, str_col)), 
 typeof(lpad(str_col, 1, str_col))
from tbl;

binary  string  string  string

Docs

Databricks LPAD | Databricks CONCAT

[georgesittas]

Can we consolidate _annotate_by_same_args and _annotate_by_args somehow? They look very similar, perhaps a flag could help?

[VaggelisD]

Can we consolidate _annotate_by_same_args and _annotate_by_args somehow? They look very similar, perhaps a flag could help?

Some parts are overlapping but the "access pattern" is different. I think as each dialect is starting to form their own annotators we should keep that logic separate from the common functions; If some boilerplate parts are getting repetitive, we should of course factor these out in time.

Regarding the Spark logic, I came up with a new set of rules for that annotator that biases STRING less now (that still remains the fallback type though). What do you think of that?

[georgesittas]

Let's test unknowns as well

[VaggelisD]

@georgesittas I did add a few test cases with UNKNOWNs in the previous commit, do you have more cases in mind?

[georgesittas]

@georgesittas I did add a few test cases with UNKNOWNs in the previous commit, do you have more cases in mind?

Ah I didn't see them, apologies. LGTM.