Only parse FROM identifier in CTE if using Hive#2241
Conversation
| } | ||
| }; | ||
| if self.parse_keyword(Keyword::FROM) { | ||
| if dialect_of!(self is HiveDialect) && self.parse_keyword(Keyword::FROM) { |
There was a problem hiding this comment.
not sure I follow the intent of the PR, is it rather that hive should support supports_from_first_select (i.e. what's the difference in behavior from hive vs other dialects)?
There was a problem hiding this comment.
I gave more context in the issue I created; I'm not knowledgeable enough about Hive to understand if it should support FROM first selects, and it is unclear (see also #235 (comment)) why this FROM token parsing was introduced here. Maybe the intent was indeed to support FROM first selects, but I'd rather not break things here so that's why I only gated this FROM parsing for Hive.
There was a problem hiding this comment.
I see, oh yeah afaict it seems to be the case that from the link the syntax was misinterpreted, the FROM doesn't belong to the CTE, rather its a variant of from_first (at least for insert statements).
I think ideally we would introduce a self.dialect.supports_from_first_insert method that hive flags and the from being set here moves from the cte to the insert statement. The latter might be out of scope for the MR however, so maybe we can just add a comment in the hive dialect method on the former mentioning that its incomplete/incorrect?
There was a problem hiding this comment.
I'm ok with this option, I can also create a follow up issue along with the comment (or just the comment if you are happy with it).
There was a problem hiding this comment.
An issue sounds great as well thanks!
|
Hi @iffyio, Just a gentle ping on this one, as it prevents properly parsing FROM first queries when used with a CTE, which can be fairly common considering FROM first queries are enabled on the generic dialect. Thanks in advance! |
| } | ||
| }; | ||
| if self.parse_keyword(Keyword::FROM) { | ||
| if dialect_of!(self is HiveDialect) && self.parse_keyword(Keyword::FROM) { |
There was a problem hiding this comment.
I see, oh yeah afaict it seems to be the case that from the link the syntax was misinterpreted, the FROM doesn't belong to the CTE, rather its a variant of from_first (at least for insert statements).
I think ideally we would introduce a self.dialect.supports_from_first_insert method that hive flags and the from being set here moves from the cte to the insert statement. The latter might be out of scope for the MR however, so maybe we can just add a comment in the hive dialect method on the former mentioning that its incomplete/incorrect?
| pipe_operators: vec![], | ||
| }; | ||
| assert_eq!(expected, ast); | ||
| assert_eq!(ast.to_string(), q); |
There was a problem hiding this comment.
I don't think this should be needed given verified_query() does this check?
There was a problem hiding this comment.
I followed the existing test_select_from_first() test for this one, but this makes sense. I removed the assertion from both tests.
|
|
||
| let ast = dialects.verified_query(q); | ||
|
|
||
| let expected = Query { |
There was a problem hiding this comment.
I wonder are we asserting something specific about the AST? It would be good to skip the assertion since the struct is quite verbose - thinking if its sufficient to only assert that the query parses and round trip. Otherwise if we can rewrite/split the assertion to only assert the components we're interested in
There was a problem hiding this comment.
I simplified to only assert the query body's projection and from clauses.
2 participants
Fixes #2236.