Skip to content

Support Snowflake/BigQuery TRIM. - #975

Merged
alamb merged 5 commits into
apache:mainfrom
getsynq:zdenko/support-snowflake-trim
Oct 6, 2023
Merged

Support Snowflake/BigQuery TRIM.#975
alamb merged 5 commits into
apache:mainfrom
getsynq:zdenko/support-snowflake-trim

Conversation

@zdenal

@zdenalzdenal commented Sep 22, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Why

We are getting errors with parsing snowflake TRIM expression including removing characters separated by comma.

Snowflake doc for TRIM

https://docs.snowflake.com/en/sql-reference/functions/trim

BigQuery doc for TRIM

https://cloud.google.com/bigquery/docs/reference/standard-sql/string_functions#trim

Example of real failing sql

WITH sub_coupons AS (
SELECT
customer_id,
subscription_id,
TRIM(coupons.value:coupon_id, '"') AS coupon_id
FROMmodel_staging.subscriptions,
TABLE(FLATTEN(PARSE_JSON(coupons))) AS coupons
)

zdenal
zdenal commented Sep 22, 2023
View reviewed changes
Comment threadsrc/ast/mod.rs
}
if let Some(characters) = trim_characters {
write!(f, ", {}", display_comma_separated(characters))?;
}

@zdenalzdenalSep 22, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

print trim characters after expression if there are

Comment threadsrc/parser/mod.rs Outdated
trim_what: Some(trim_what),
trim_characters: None,
})
} else if self.consume_token(&Token::Comma) && dialect_of!(self is SnowflakeDialect) {

@zdenalzdenalSep 22, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

no FROM (by doc Snowflake doesn't support FROM syntax) andSnowflakeand next isComma -> parse characters

Comment threadtests/sqlparser_common.rs
Box::new(PostgreSqlDialect {}),
Box::new(MsSqlDialect {}),
Box::new(AnsiDialect {}),
//Box::new(SnowflakeDialect {}),

@zdenalzdenalSep 22, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

make sure it is still failing on others than snowflake.

@zdenal
zdenal marked this pull request as ready for review September 22, 2023 11:58
alamb
alamb approved these changes Oct 2, 2023
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great -- thank you @zdenal

@alamb

alamb commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

There appear to be some small CI errors -- @zdenal can you please fix them?

@zdenalzdenal changed the title Support Snowflake TRIM.Support Snowflake/BigQuery TRIM.Oct 4, 2023
@zdenal

zdenal commented Oct 4, 2023

Copy link
Copy Markdown
ContributorAuthor

@alamb RDY again. I have also added BigQuery to support this syntax (link to BigQuery TRIM doc attached in PR description).

@zdenal
zdenal requested a review from alambOctober 4, 2023 06:43
alamb
alamb approved these changes Oct 5, 2023
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @zdenal for your contribution. I had one small comment and then I think this PR is ready to go

Comment threadsrc/parser/mod.rs Outdated
trim_characters: None,
})
} else if self.consume_token(&Token::Comma)
&& dialect_of!(self is SnowflakeDialect | BigQueryDialect)

@alambalambOct 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the general pattern for this crate is to also include GenericDialect so it is a superset (as much as possible) of all dialects

@zdenalzdenalOct 5, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@alamb .. added GenericDialect

@alamb

alamb commented Oct 5, 2023

Copy link
Copy Markdown
Contributor

The CI failures do not appear to be related to this PR -- see #995

I believe if you merge / rebase this branch with main the CI will pass cleanly

@zdenal

zdenal commented Oct 5, 2023

Copy link
Copy Markdown
ContributorAuthor

@alamb RDY again.

@zdenal
zdenal requested a review from alambOctober 5, 2023 19:50
alamb
alamb approved these changes Oct 5, 2023
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @zdenal

@coveralls

coveralls commented Oct 5, 2023

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 6423740010

  • 50 of 50(100.0%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 87.354%
Files with Coverage ReductionNew Missed Lines%
src/ast/mod.rs178.86%
TotalsCoverage Status
Change from base Build 6423634893:0.03%
Covered Lines:16778
Relevant Lines:19207
💛 - Coveralls

@alamb
alamb merged commit 83cb734 into apache:mainOct 6, 2023
serprex pushed a commit to serprex/sqlparser-rs that referenced this pull request Nov 6, 2023
@zdenal@serprex
Support Snowflake/BigQuery TRIM. (apache#975)
9f45634
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zdenal@alamb@coveralls