-
Notifications
You must be signed in to change notification settings - Fork 605
Support NestedJoin
with an alias
#551
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
Conversation
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~ |
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 🤔 |
@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 |
6575513
to
581b9a2
Compare
@alamb fixed snowflake's test case, add 2 new test cases in |
Pull Request Test Coverage Report for Build 2792676017
💛 - Coveralls |
NestedJoin(Box<TableWithJoins>), | ||
NestedJoin { | ||
table_with_joins: Box<TableWithJoins>, | ||
alias: Option<TableAlias>, |
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.
👍
|
||
// natural join with alias | ||
assert_eq!( | ||
only(&verified_only_select("SELECT * FROM t1 NATURAL JOIN t2 AS t3").from).joins, |
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.
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?
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.
it alias t2 to t3
but not alias to the entire NaturalJoin
.
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.
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
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.
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"; |
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.
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( |
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.
nice
tests/test_utils/mod.rs
Outdated
} | ||
|
||
#[macro_export] | ||
macro_rules! nest_with_alias { |
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.
Since this macro is only invoked in a single place I can see, what do you think about inlining it instead?
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.
done
Thanks @waitingkuo -- this is looking good |
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.
Thanks again @waitingkuo !
closes #537 , apache/datafusion#2867
origin nestedjoin doesn't have alias. this pr add alias for nestedjoin