Skip to content

support PIVOT table syntax#836

Merged
alamb merged 1 commit into
apache:mainfrom
pawel-big-lebowski:snowflake/pivot-table
Mar 26, 2023
Merged

support PIVOT table syntax#836
alamb merged 1 commit into
apache:mainfrom
pawel-big-lebowski:snowflake/pivot-table

Conversation

@pawel-big-lebowski

Copy link
Copy Markdown
Contributor

PIVOT TABLE syntax is available in Snowflake.
This PR introduces PIVOT support within sqlparser-rs.

@coveralls

coveralls commented Mar 15, 2023

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 4466498674

  • 68 of 75 (90.67%) changed or added relevant lines in 3 files are covered.
  • 12 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 86.138%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 19 20 95.0%
tests/sqlparser_common.rs 30 31 96.77%
src/ast/query.rs 19 24 79.17%
Files with Coverage Reduction New Missed Lines %
src/parser.rs 12 82.95%
Totals Coverage Status
Change from base Build 4456045946: 0.01%
Covered Lines: 13646
Relevant Lines: 15842

💛 - Coveralls

@pawel-big-lebowski

Copy link
Copy Markdown
Contributor Author

Failing lint should be resolved within #835

Comment thread src/ast/query.rs
alias: Option<TableAlias>,
},
/// Represents PIVOT operation on a table.
/// For example `FROM monthly_sales PIVOT(sum(amount) FOR MONTH IN ('JAN', 'FEB'))`

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.

Could you add a link to the Snowflake docs (https://docs.snowflake.com/en/sql-reference/constructs/pivot) in here?

Comment thread tests/sqlparser_common.rs
#[test]
fn parse_pivot_table() {
let sql = concat!(
"SELECT * FROM monthly_sales AS a ",

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.

  • can you add a test case without the AS component?
  • can you add a roundtrip test (so we can verify the Display() implementation works too)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  • There is below a variable sql_without_table_alias that tests this.
  • Will add the roundtrip as it is missing.

Comment thread src/parser.rs Outdated
self.expect_token(&Token::LParen)?;
let function_name = match self.next_token().token {
Token::Word(w) => Ok(w.value),
_ => self.expected("a function name", self.peek_token()),

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.

maybe "an aggregate function" in the error message?

Comment thread src/parser.rs Outdated
}?;
let function = self
.parse_function(ObjectName(vec![Ident::new(function_name)]))
.unwrap();

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.

shouldn't we propagate the error here instead of unwrapping?

Comment thread src/parser.rs Outdated
.parse_function(ObjectName(vec![Ident::new(function_name)]))
.unwrap();
self.expect_keyword(Keyword::FOR)?;
let mut value_column = vec![self.parse_identifier()?];

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.

why not use self.parse_object_name()?

Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com>

@alamb alamb 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 @pawel-big-lebowski -- looks good to me. Thanks for the assist @ankrgyl

@alamb alamb merged commit 29dea5d into apache:main Mar 26, 2023
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.

4 participants