Fixing alter table multi partition #657
Conversation
bitemyapp
force-pushed
the
bitemyapp/fixing-alter-table-multi-partition
branch
from
7c74aa9 to
879f529
Compare
Pull Request Test Coverage Report for Build 3206495452
💛 - Coveralls |
bitemyapp
force-pushed
the
bitemyapp/fixing-alter-table-multi-partition
branch
from
879f529 to
03d778d
Compare
|
Buhhhh forgot to |
bitemyapp
force-pushed
the
bitemyapp/fixing-alter-table-multi-partition
branch
from
03d778d to
d5c9854
Compare
|
Switched the match expression to |
|
@bitemyapp please add information about sources and references. Only putting the query there makes me search for 5 dialects at least before making sure that's valid |
| self.expect_token(&Token::LParen)?; | ||
| let partitions = self.parse_comma_separated(Parser::parse_expr)?; | ||
| self.expect_token(&Token::RParen)?; | ||
| if self.peek_keyword(Keyword::PARTITION) { |
There was a problem hiding this comment.
Is this hive-specific? It looks like so, right?
Could you please guarantee that the dialect is Hive or Generic before accepting more than one partition?
There was a problem hiding this comment.
To be frank I'd rather not. It's logic that shouldn't break the parser for anyone parsing a different dialect and it's abundantly clear that this library is not attempting to be a validation/standards-verification parser. If you have a contrary example where this breaks a parse that should've worked in another context I'll see what I can do to deal with that. I don't think that exists yet though. I lost a lot of time to being confused by the dialect specialization and I'd rather not add more of it.
There isn't any dialect specialization happening anywhere else near this part of the parser that I can recall either.
There was a problem hiding this comment.
Following are three examples of specialization by dialect inside the code.
My problem is that if you can validate a specific structure for a dialect and know which dialect supports that pattern, you could parse based on the dialect. Since your PR is related to an extension, not a completely different of doing the parse, I don't see why it is a problem to allow Hive and Generic to have that syntax, while other dialects not.
There was a problem hiding this comment.
In general, I think the crate takes the approach of if we can parse the syntax unambiguously we'll accept it even if certain syntax would not be accepted by the various dialects. This is definitely not totally consistent as @bitemyapp has noticed
I tend to agree that in this case we aren't adding any ambiguity so this implementation is fine
bitemyapp
force-pushed
the
bitemyapp/fixing-alter-table-multi-partition
branch
from
d5c9854 to
fe55986
Compare
|
Removed |
| self.expect_token(&Token::LParen)?; | ||
| let partitions = self.parse_comma_separated(Parser::parse_expr)?; | ||
| self.expect_token(&Token::RParen)?; | ||
| if self.peek_keyword(Keyword::PARTITION) { |
There was a problem hiding this comment.
In general, I think the crate takes the approach of if we can parse the syntax unambiguously we'll accept it even if certain syntax would not be accepted by the various dialects. This is definitely not totally consistent as @bitemyapp has noticed
I tend to agree that in this case we aren't adding any ambiguity so this implementation is fine
|
I'll wait for the token parsing thing to merge and we can then merge this one |
bitemyapp
force-pushed
the
bitemyapp/fixing-alter-table-multi-partition
branch
from
fe55986 to
438c515
Compare
bitemyapp
force-pushed
the
bitemyapp/fixing-alter-table-multi-partition
branch
from
438c515 to
2016b64
Compare
|
CI is failing so marking as draft |
|
Closing this PR as it is more than 6 months old without activity. Please reopen or make another PR if you plan to keep working on this. |
Fixes examples like this:
This includes the backtick commit by necessity but we can merge them all at once or the backtick commit separately, your choice.