[SPARK-20931][SQL] ABS function support string type. #18153
Conversation
| case dt: DecimalType => | ||
| defineCodeGen(ctx, ev, c => s"$c.abs()") | ||
| case dt: NumericType => | ||
| defineCodeGen(ctx, ev, c => s"(${ctx.javaType(dt)})(java.lang.Math.abs($c))") | ||
| case dt: StringType => | ||
| defineCodeGen(ctx, ev, c => s"java.lang.Math.abs(Double.valueOf($c.toString()))") |
There was a problem hiding this comment.
How should it work when a given string includes non-numerical character?
e.g. https://zohoreportsapi.wiki.zoho.com/MySQL-Math-Functions.html
There was a problem hiding this comment.
NumberFormatException will throw, Hive does not support non-numerical character also.
There was a problem hiding this comment.
MySQL support it, do we need to support this feature?
There was a problem hiding this comment.
I would like to hear @gatorsmile 's opinion. Which specification should Spark support?
There was a problem hiding this comment.
Thanks for ping me. To be consistent with the other mathematic expressions in Spark SQL, let us just convert it to Double.
|
Test build #77571 has finished for PR 18153 at commit
|
|
Test build #77576 has started for PR 18153 at commit |
|
Jenkins, retest this please |
|
Test build #77583 has finished for PR 18153 at commit
|
| case dt: DecimalType => | ||
| defineCodeGen(ctx, ev, c => s"$c.abs()") | ||
| case dt: NumericType => | ||
| defineCodeGen(ctx, ev, c => s"(${ctx.javaType(dt)})(java.lang.Math.abs($c))") | ||
| case dt: StringType => |
There was a problem hiding this comment.
Could we move the type promotion to the rule PromoteStrings?
|
Test build #78170 has finished for PR 18153 at commit
|
|
LGTM pending Jenkins. |
|
Test build #78182 has finished for PR 18153 at commit
|
|
Thanks! Merging to master. |
What changes were proposed in this pull request?
ABS function support string type. Hive/MySQL support this feature.
Ref: https://github.com/apache/hive/blob/4ba713ccd85c3706d195aeef9476e6e6363f1c21/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAbs.java#L93
How was this patch tested?
unit tests