[SPARK-29854]lpad and rpad built in function should throw Error or Exception for invalid length value

[07ARB]

What changes were proposed in this pull request?

lpad and rpad built in function should throw Error or Exception for invalid length value Instead of empty string.

Why are the changes needed?

We should throw Error or Exception message, if user trying to perform LPAD/RPAD operation using invalid argument.

SELECT lpad('hi', 'ankit', '?') => ""   <previous behaviours>
SELECT lpad('hi', 'ankit', '?') => "Error in query: Invalid argument, TypeCheckFailure(argument 2 requires int type, however, ''ankit'' is of string type.);"   <After this PR>

SELECT rpad('hi', 'raj', '?') => ""   <previous behaviours>
SELECT lpad('hi', 'raj', '?') => "Error in query: Invalid argument, TypeCheckFailure(argument 2 requires int type, however, ''raj'' is of string type.);"   <After this PR>

Does this PR introduce any user-facing change?

YES

How was this patch tested?

Added new test case to check invalid length value.

[07ARB]

@cloud-fan and @srowen , please review this PR.

[maropu]

ok to test

[07ARB]

@maropu , Thank you

[maropu]

But, there are many functions having the same behaivour? e.g.,

scala> sql("SELECT substring_index('www.apache.org', '.', 'Must be integer')").show()
+----------------------------------------------------------------+
|substring_index(www.apache.org, ., CAST(Must be integer AS INT))|
+----------------------------------------------------------------+
|                                                            null|
+----------------------------------------------------------------+

Actually, implicit casts do so.

[07ARB]

yes , we need to discuss whether we need to handle or not.
but what i feel, we should show error message, so that end user will get some meaningful information.

[maropu]

Yea, I think this behaivour is weird, but we just follow the hive behviour;
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala#L948-L950

We are planning to support ansi-compatible type coercion, so I think this issue will be resolved if that supported. cc: @gengliangwang @cloud-fan

[SparkQA]

Test build #115885 has finished for PR 27024 at commit 2793ac6.

• This patch fails due to an unknown error code, -9.
• This patch merges cleanly.
• This patch adds no public classes.

[07ARB]

in Hive : SELECT lpad('hihhhhhhhhhhhhhhhhhhhhhhh', 'Expected int', '????????????');
Error: Error while compiling statement: FAILED: SemanticException [Error 10016]: Line 1:67 Argument type mismatch ''????????????'': lpad only takes INT/SHORT/BYTE types as 2-ths argument, got DECIMAL (state=42000,code=10016)

[maropu]

I just meant not the lpad behaivour but the type coercion behaivour;

hive> SELECT '1' + 1;
2.0

If you want to fix this, I think we need to update the implicit cast logics instead of the lpad logic.

[07ARB]

oh , ok got it.

[github-actions]

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

[cloud-fan]

This behavior LGTM under the ansi mode. We should also update docs/sql-ref-ansi-compliance.md

[AmplabJenkins]

Can one of the admins verify this patch?

[maropu]

@07ARB Are you still here? Could you update this PR based on the @cloud-fan suggestion above?