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

Support NestedJoin with an alias #551

Merged
merged 4 commits into from Aug 3, 2022
Merged

Conversation

waitingkuo
Copy link
Contributor

closes #537 , apache/datafusion#2867

origin nestedjoin doesn't have alias. this pr add alias for nestedjoin

@waitingkuo
Copy link
Contributor Author

help wanted.

this pr break this test

    let res = snowflake_and_generic().parse_sql_statements("SELECT * FROM (a NATURAL JOIN b) c");
    assert_eq!(
        ParserError::ParserError("Expected end of statement, found: c".to_string()),
        res.unwrap_err()
    );

Could anyone tell me how to deal with this?

thanks~

@alamb
Copy link
Collaborator

alamb commented Aug 3, 2022

this pr break this test

That test is validating that there is a parser error, right @waitingkuo ?

It looks like it was added as part of #260 from @eyalleshem -- perhaps there is some information there.

Perhaps we can update the test with the new behavior as the point of this PR is to support aliases 🤔

@waitingkuo
Copy link
Contributor Author

@alamb the original test case is to test whether the parser could generate the right error. Since that error is solved in error, i'll update that case

@waitingkuo waitingkuo marked this pull request as ready for review August 3, 2022 20:14
@waitingkuo
Copy link
Contributor Author

@alamb fixed snowflake's test case, add 2 new test cases in sqlparser_common

@coveralls
Copy link

coveralls commented Aug 3, 2022

Pull Request Test Coverage Report for Build 2792676017

  • 49 of 56 (87.5%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 89.91%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ast/query.rs 6 8 75.0%
src/parser.rs 9 14 64.29%
Totals Coverage Status
Change from base Build 2789692749: -0.02%
Covered Lines: 9053
Relevant Lines: 10069

💛 - Coveralls

NestedJoin(Box<TableWithJoins>),
NestedJoin {
table_with_joins: Box<TableWithJoins>,
alias: Option<TableAlias>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍


// natural join with alias
assert_eq!(
only(&verified_only_select("SELECT * FROM t1 NATURAL JOIN t2 AS t3").from).joins,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is an interesting one -- it adds the alias to the entire NaturalJoin rather than aliasing t2 to t3 on one side of the input.

Is that what other implementations do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it alias t2 to t3 but not alias to the entire NaturalJoin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps i should delete this test? i thought at we need to protect the case that it incorrectly alias the whole natural join to t3

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is a good test and that we should leave it in

@@ -3519,6 +3532,15 @@ fn parse_join_nesting() {
from.joins,
vec![join(nest!(nest!(nest!(table("b"), table("c")))))]
);

let sql = "SELECT * FROM (a NATURAL JOIN b) AS c";
Copy link
Collaborator

Choose a reason for hiding this comment

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

this makes sense to me 👍

assert_eq!(
ParserError::ParserError("Expected end of statement, found: c".to_string()),
res.unwrap_err()
snowflake_and_generic().one_statement_parses_to(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

}

#[macro_export]
macro_rules! nest_with_alias {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this macro is only invoked in a single place I can see, what do you think about inlining it instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@alamb
Copy link
Collaborator

alamb commented Aug 3, 2022

Thanks @waitingkuo -- this is looking good

Copy link
Collaborator

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks again @waitingkuo !

@alamb alamb changed the title NestedJoin with alias Support NestedJoin with an alias Aug 3, 2022
@alamb alamb merged commit 076b587 into sqlparser-rs:main Aug 3, 2022
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.

Nested alias doesn't work
3 participants