goldmedal commented on code in PR #1541:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1541#discussion_r1856938487
##########
src/parser/mod.rs:
##########
@@ -2935,12 +2935,23 @@ impl<'a> Parser<'a> {
})
} else if Token::LBracket == tok {
if dialect_of!(self is PostgreSqlDialect | DuckDbDialect |
GenericDialect) {
- self.parse_subscript(expr)
+ let expr = self.parse_multi_dim_subscript(expr)?;
+ if self.dialect.support_period_map_access_key() {
+ self.parse_map_access(expr, vec![])
+ } else {
+ Ok(expr)
+ }
Review Comment:
> The first option to merge subscript into mapaccess sounds reasonable! I'm
thinking we could skip the rename at least to start with to keep the breakage
minimal and I'm imagining it shouldn't be as large of a change in that case,
wdyt?
I think removing `Subscript` causes more breakage than removing `MapAccess`.
🤔
Do you know how many downstream projects use `MapAccess`? I found that
`DataFusion` hasn’t implemented it, but `Subscript` is used to handle array
syntax. I'm not sure which one is better.
I drafted a version for option 2:
https://github.com/apache/datafusion-sqlparser-rs/pull/1551.
It still has some issues with `Expr::Method` parsing, but I think it
preserves `Subscript` and avoids a significant breaking change for downstream
projects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
