Skip to content
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

Add TRIM functionality #88

Merged
merged 18 commits into from
Nov 4, 2021
Merged

Conversation

jycor
Copy link

@jycor jycor commented Nov 2, 2021

Changed sql.y to recognize keywords TRIM, LEADING, TRAILING, and BOTH

@jycor jycor requested a review from zachmu as a code owner November 2, 2021 22:53
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, needs a little expanasion

type TrimExpr struct {
Str Expr
Pattern Expr
Dir Expr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should make this a string and define const values for it (LEADING, TRAILING, BOTH)

{
$$ = &TrimExpr{Pattern: NewStrVal([]byte(" ")), Str: $3, Dir: NewStrVal([]byte("b"))}
}
| TRIM openb STRING FROM value_expression closeb
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does MySQL require this to a string literal, or can you use other expressions? Test and seee

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it can take expressions:

mysql> SELECT TRIM(LEADING concat('x', 'x') FROM 'xxxbarxxx');
+-------------------------------------------------+
| TRIM(LEADING concat('x', 'x') FROM 'xxxbarxxx') |
+-------------------------------------------------+
| xbarxxx                                         |
+-------------------------------------------------+

@jycor jycor changed the title James/fix 2297 implement string functions Add TRIM functionality Nov 3, 2021
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG, just a couple small issues and you can merge

@@ -2643,8 +2643,8 @@ func TestKeywords(t *testing.T) {
input: "select /* share and mode as cols */ share, mode from t where share = 'foo'",
output: "select /* share and mode as cols */ `share`, `mode` from t where `share` = 'foo'",
}, {
input: "select /* unused keywords as cols */ write, virtual from t where trailing = 'foo'",
output: "select /* unused keywords as cols */ `write`, `virtual` from t where `trailing` = 'foo'",
input: "select /* unused keywords as cols */ write, virtual from t where varcharacter = 'foo'",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to this test case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment still applies, you changed this test case and should put it back

testCases := []parseTest{
{
input: `SELECT TRIM("foo")`,
output: `select trim('foo', ' ', b) from dual`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So generally speaking, the Format function should produce output which is itself parseable, and this won't

Unless you have a good reason not to do it this way instead, you should change the Format function to return something similar to the input

type TrimExpr struct {
Str Expr
Pattern Expr
Dir string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare some exported constants for this value

@@ -3889,6 +3890,26 @@ function_call_keyword:
{
$$ = &SubstrExpr{StrVal: NewStrVal($3), From: $5, To: $7}
}
| TRIM openb value_expression closeb
{
$$ = &TrimExpr{Pattern: NewStrVal([]byte(" ")), Str: $3, Dir: "b"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the exported constants here rather than string literals

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -2643,8 +2643,8 @@ func TestKeywords(t *testing.T) {
input: "select /* share and mode as cols */ share, mode from t where share = 'foo'",
output: "select /* share and mode as cols */ `share`, `mode` from t where `share` = 'foo'",
}, {
input: "select /* unused keywords as cols */ write, virtual from t where trailing = 'foo'",
output: "select /* unused keywords as cols */ `write`, `virtual` from t where `trailing` = 'foo'",
input: "select /* unused keywords as cols */ write, virtual from t where varcharacter = 'foo'",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment still applies, you changed this test case and should put it back

@jycor jycor merged commit 4cc2d2c into main Nov 4, 2021
@Hydrocharged Hydrocharged deleted the james/fix-2297-implement-string-functions branch October 13, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants