-
Notifications
You must be signed in to change notification settings - Fork 28.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-20754][SQL][FOLLOWUP] Add Function Alias For MOD/POSITION. #18206
Conversation
Test build #77748 has finished for PR 18206 at commit
|
@wangyum Could you resolve the conflicts? Thanks! |
@@ -15,3 +15,6 @@ select replace('abc', 'b'); | |||
|
|||
-- uuid | |||
select length(uuid()), (uuid() <> uuid()); | |||
|
|||
-- position | |||
select position('bar', 'foobarbar'), position('bar', 'foobarbar', 5), position(null, 'foobarbar'), position('aaads', null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The syntax of position is POSITION(substr IN str)
. It is different from LOCATE
. You need to change the parser to support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check what we did for remainder and the others in Parser supports.
@@ -70,3 +70,6 @@ select ceiling(1234567890123456); | |||
select floor(0); | |||
select floor(1); | |||
select floor(1234567890123456); | |||
|
|||
-- mod | |||
select mod(7, 2), mod(7, 0), mod(0, 2), mod(7, null), mod(null, 2), mod(null, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update the function description of Remainder
. The example does not show this syntax.
…ition Conflicts: sql/core/src/test/resources/sql-tests/inputs/operators.sql sql/core/src/test/resources/sql-tests/results/operators.sql.out
Test build #77993 has finished for PR 18206 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. Left a few minor comments.
@@ -720,6 +721,7 @@ nonReserved | |||
| SET | RESET | |||
| VIEW | REPLACE | |||
| IF | |||
| POSITION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the keyword to TableIdentifierParserSuite
@@ -574,6 +574,7 @@ primaryExpression | |||
| identifier #columnReference | |||
| base=primaryExpression '.' fieldName=identifier #dereference | |||
| '(' expression ')' #parenthesizedExpression | |||
| POSITION '(' valueExpression IN valueExpression ')' #position |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move it to the line 566
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
POSITION '(' substr=valueExpression IN str=valueExpression ')'
@@ -851,6 +853,8 @@ IGNORE: 'IGNORE'; | |||
|
|||
IF: 'IF'; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this space.
> SELECT _FUNC_('bar', 'foobarbar', 5); | ||
7 | ||
> SELECT POSITION('bar' in 'foobarbar'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in
-> IN
* Create a Position expression. | ||
*/ | ||
override def visitPosition(ctx: PositionContext): Expression = withOrigin(ctx) { | ||
StringLocate(expression(ctx.valueExpression(0)), expression(ctx.valueExpression(1)), Literal(1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new StringLocate(expression(ctx.substr), expression(ctx.str))
Test build #78014 has finished for PR 18206 at commit
|
LGTM |
Thanks! Merging to master. |
## What changes were proposed in this pull request? apache#18106 Support TRUNC (number), We should also add function alias for `MOD `and `POSITION`. `POSITION(substr IN str) `is a synonym for `LOCATE(substr,str)`. same as MySQL: https://dev.mysql.com/doc/refman/5.7/en/string-functions.html#function_position ## How was this patch tested? unit tests Author: Yuming Wang <[email protected]> Closes apache#18206 from wangyum/SPARK-20754-mod&position.
What changes were proposed in this pull request?
#18106 Support TRUNC (number), We should also add function alias for
MOD
andPOSITION
.POSITION(substr IN str)
is a synonym forLOCATE(substr,str)
. same as MySQL: https://dev.mysql.com/doc/refman/5.7/en/string-functions.html#function_positionHow was this patch tested?
unit tests