alamb commented on code in PR #1551:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1551#discussion_r1874012439
##########
tests/sqlparser_common.rs:
##########
@@ -10299,20 +10324,38 @@ fn parse_map_access_expr() {
Box::new(ClickHouseDialect {}),
]);
let expr = dialects.verified_expr(sql);
- let expected = Expr::MapAccess {
- column: Expr::Identifier(Ident::new("users")).into(),
- keys: vec![
- MapAccessKey {
- key: Expr::UnaryOp {
+ let expected = Expr::CompoundExpr {
+ root: Box::new(Expr::Identifier(Ident::with_span(
+ Span::new(Location::of(1, 1), Location::of(1, 6)),
+ "users",
+ ))),
+ chain: vec![
+ AccessField::Subscript(Subscript::Index {
+ index: Expr::UnaryOp {
op: UnaryOperator::Minus,
expr: Expr::Value(number("1")).into(),
},
- syntax: MapAccessSyntax::Bracket,
- },
- MapAccessKey {
- key: call("safe_offset", [Expr::Value(number("2"))]),
- syntax: MapAccessSyntax::Bracket,
- },
+ }),
+ AccessField::Subscript(Subscript::Index {
Review Comment:
this seems like a very reasonable structure
##########
src/ast/mod.rs:
##########
@@ -1094,6 +1053,25 @@ impl fmt::Display for Subscript {
}
}
+/// The contents inside the `.` in an access chain.
Review Comment:
Maybe we could add a doc link to `Expr::CompoundExpr` like
```suggestion
/// An element of a [`Expr::CompoundExpr`]
```
##########
src/ast/mod.rs:
##########
@@ -624,6 +590,12 @@ pub enum Expr {
Identifier(Ident),
/// Multi-part identifier, e.g. `table_alias.column` or `schema.table.col`
CompoundIdentifier(Vec<Ident>),
+ /// Multi-part Expression accessing. It's used to represent an access
chain from a root expression.
Review Comment:
```suggestion
/// Multi-part expression access.
///
/// This structure represents an access chain in structured / nested
types
/// such as maps, arrays, and lists
```
##########
src/ast/mod.rs:
##########
@@ -624,6 +590,12 @@ pub enum Expr {
Identifier(Ident),
/// Multi-part identifier, e.g. `table_alias.column` or `schema.table.col`
CompoundIdentifier(Vec<Ident>),
+ /// Multi-part Expression accessing. It's used to represent an access
chain from a root expression.
+ /// e.g. `expr[0]`, `expr[0][0]`, or `expr.field1.filed2[1].field3`, ...
+ CompoundExpr {
Review Comment:
Also, it seems to me that the name `Compound` is somewhat misleading in this
context
It isn't a compound expression per se, it seems more like an a compoound
access / multi level access
What do you think about names like `CompoundAccess` ?(that would also fit
nicely with @iffyio 's suggestion at
https://github.com/apache/datafusion-sqlparser-rs/pull/1551/files#r1871788945
for `AccessExpr`. Maybe something like 🤔
```rust
...
CompoundAccess {
root: Box<Expr>,
accesses: Vec<AccessExprs>
...
```
I am not sure I really like `accesses` (too many `s` 🤔 )
Maybe `CompoundFieldAccess` 🤔
##########
tests/sqlparser_common.rs:
##########
@@ -2995,6 +2995,31 @@ fn parse_window_function_null_treatment_arg() {
);
}
+#[test]
+fn test_compound_expr() {
+ let supported_dialects = TestedDialects::new(vec![
+ Box::new(GenericDialect {}),
+ Box::new(DuckDbDialect {}),
+ Box::new(BigQueryDialect {}),
+ ]);
+ let sqls = [
+ "SELECT abc[1].f1 FROM t",
+ "SELECT abc[1].f1.f2 FROM t",
Review Comment:
this is pretty cool
##########
src/ast/mod.rs:
##########
@@ -1289,12 +1267,19 @@ impl fmt::Display for Expr {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
Expr::Identifier(s) => write!(f, "{s}"),
- Expr::MapAccess { column, keys } => {
- write!(f, "{column}{}", display_separated(keys, ""))
- }
Expr::Wildcard(_) => f.write_str("*"),
Expr::QualifiedWildcard(prefix, _) => write!(f, "{}.*", prefix),
Expr::CompoundIdentifier(s) => write!(f, "{}",
display_separated(s, ".")),
+ Expr::CompoundExpr { root, chain } => {
+ write!(f, "{}", root)?;
+ for field in chain {
+ match field {
+ AccessField::Expr(expr) => write!(f, ".{}", expr)?,
Review Comment:
I would have expected the `,` or the `[]` be in the `AccessField` so it can
be self-displaying
##########
src/ast/mod.rs:
##########
@@ -624,6 +590,12 @@ pub enum Expr {
Identifier(Ident),
/// Multi-part identifier, e.g. `table_alias.column` or `schema.table.col`
CompoundIdentifier(Vec<Ident>),
+ /// Multi-part Expression accessing. It's used to represent an access
chain from a root expression.
+ /// e.g. `expr[0]`, `expr[0][0]`, or `expr.field1.filed2[1].field3`, ...
Review Comment:
I think it would be awesome to update this comment with the information from
this PR's description:
--
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
