Consolidate `MapAccess`, and `Subscript` into `CompoundExpr` to handle the complex field access chain #1551

goldmedal · 2024-11-25T16:33:17Z

Description

This PR introduces a new Expr, Expr::CompoundExpr, which handles the complex field access chain like a.b[1].c or func()[1]. As the previous discussion #1541 (comment), I found we have many similar expressions for the similar purpose (e.g. MapAccess, Subscript, JsonAcess, ...). CompoundExpr tries to represent a structure for all of them. As the suggestion from @iffyio (#1541 (comment)), CompoundExpr is a linear structure, it has a root expression and a list of access fields, instead of a nested representation.

    /// 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 {
        root: Box<Expr>,
        chain: Vec<AccessField>,
    },

To avoid too many breaking changes, this PR only replaces Expr::MapAccess and Expr::Subscript with Expr::CompoundExpr. Potentially, it can represent more things.

Array

A 1-dim array a[1] will be represented like

CompoundExpr(Ident('a'), vec![Subscript(1)]

A 2-dim array a[1][2] will be represented like

CompoundExpr(Ident('a'), vec![Subscript(1), Subscript(2)]

Map or Struct (Bracket-style)

A map a['fied1'] will be represented like

CompoundExpr(Ident('a'), vec![Subscript('field')]

A 2-dim map a['field1']['field2'] will be represented like

CompoundExpr(Ident('a'), vec![Subscript('field1'), Subscript('field2')]

Struct (Period-style) (only effect when the chain contains both `subscript` and `expr`)

A struct access a[field1].field2 will be represented

CompoundExpr(Ident('a'), vec![Subscript('field1'), Ident('field2')]

If a struct access likes a.field1.field2, it will be represented by CompoundIdentifer([a, field1, field2])

Breaking Change

Expr::Subscript and Expr::MapAccess are removed.
The result of the array, map, or struct will be changed.

Additional context

Enhance the nested type access for Generic and DuckDB dialect #1541 (comment)

goldmedal · 2024-12-04T18:10:02Z

@iffyio This PR is ready for review. Could you take a look at it? Thanks 🙇
This PR involves some breaking changes to the array syntax. I'd appreciate it if @alamb could check it as well. 🙇

src/ast/mod.rs

src/lib.rs

src/parser/mod.rs

iffyio · 2024-12-06T14:33:01Z

src/parser/mod.rs

+                } else {
+                    let mut expr = self.parse_function(ObjectName(id_parts))?;
+                    // consume all period if it's a method chain
+                    if self.dialect.supports_methods() {


try_parse_method already does the if self.dialect.supports_methods() check so that we should be able to skip the if condition?

No, if this condition was removed, the test for method parsing will fail:

parse_method_expr parse_method_select

Because the following parse_compound_expr will try to consume all the dots for an expression, we need to parse the method here to avoid consume ( in parse_compound_expr.

Oh, to clarify what I meant was that try_parse_method does this already

if !self.dialect.supports_methods() { return Ok(expr); }

So that this can be simplified as following (i.e without the extra if self.dialect.supports_methods())

expr = self.try_parse_method(expr)?;

Indeed. It's more simple. Thanks!

src/parser/mod.rs

alamb

Thanks @goldmedal and @iffyio

I didn't study the parser changes closely, but I reviewed the Expr representation and left some comments.

In terms of the potential downstream impacts (and avoiding regressions), one thing that I suggest to help evaluate the potential downstream impact is of this trying it out in DataFusion and see if it works / passes all the tests;

You could potentially use apache/datafusion#13546 as a base to test with

src/ast/mod.rs

tests/sqlparser_common.rs

goldmedal · 2024-12-09T16:59:35Z

Thanks, @alamb and @iffyio for reviewing.

In terms of the potential downstream impacts (and avoiding regressions), one thing that I suggest to help evaluate the potential downstream impact is of this trying it out in DataFusion and see if it works / passes all the tests;

You could potentially use apache/datafusion#13546 as a base to test with

Sounds good 👍 I'll test this PR based on it tomorrow.

goldmedal · 2024-12-11T17:31:16Z

Thanks, @alamb and @iffyio for reviewing.

In terms of the potential downstream impacts (and avoiding regressions), one thing that I suggest to help evaluate the potential downstream impact is of this trying it out in DataFusion and see if it works / passes all the tests;
You could potentially use apache/datafusion#13546 as a base to test with

Sounds good 👍 I'll test this PR based on it tomorrow.

Created apache/datafusion#13734 for testing. The good news is that It only impacts a few parts of the planner and the unparser 👍

alamb

Thanks @goldmedal -- this looks good to me. 🙏

alamb · 2024-12-11T22:37:30Z

Given the potential for non trivial downstream conflicts due to this change (look at the list of conflicts it has already collected) I would like to merge it in right after we have done the next release

Release sqlparser-rs version 0.53.0 / sqlparser_derive 0.3.0 #1517

iffyio

Thanks @goldmedal! Left some minor comments, the changes look good to me!

iffyio · 2024-12-13T12:40:24Z

src/parser/mod.rs

@@ -3167,42 +3287,23 @@ impl<'a> Parser<'a> {

    pub fn parse_map_access(&mut self, expr: Expr) -> Result<Expr, ParserError> {
        let key = self.parse_expr()?;
+        let result = match key {


hmm I was initially wondering about this match since it looked incorrect to restrict the key expression type. e.g. this BigQuery test case. But then I realised that test case is passing because it now takes a different codepath via parse_compound_field_access. And so I wonder if there are any scenarios that rely on this method anymore, if there aren't it seems like we might be able to remove it entirely?

You're right. We can remove it entirely.

fixed by 5c54d1b

src/parser/mod.rs

iffyio · 2024-12-13T12:52:23Z

src/parser/mod.rs

+                } else {
+                    let mut expr = self.parse_function(ObjectName(id_parts))?;
+                    // consume all period if it's a method chain
+                    if self.dialect.supports_methods() {


Oh, to clarify what I meant was that try_parse_method does this already

if !self.dialect.supports_methods() { return Ok(expr); }

So that this can be simplified as following (i.e without the extra if self.dialect.supports_methods())

expr = self.try_parse_method(expr)?;

iffyio · 2024-12-13T13:25:16Z

src/parser/mod.rs

+        }
+    }
+
+    fn exprs_to_idents(root: &Expr, fields: &[AccessExpr]) -> Option<Vec<Ident>> {


Could we add a description of what the function does?

iffyio · 2024-12-13T13:27:22Z

src/parser/mod.rs

+        }
+    }
+
+    fn exprs_to_idents(root: &Expr, fields: &[AccessExpr]) -> Option<Vec<Ident>> {


since the caller wouldn't need the copy of root and fields it would be nice to avoid the cloning? maybe something like this could work?

fn exprs_to_idens(root: Expr, fields: Vec<AccessExprs>) -> Result<Vec<Ident>, (Expr, Vec<AccessExprs>)>

so that if no conversion was made the caller gets back the same values they provided?

Nice catch. I think we can save the cloning by separating it into two functions. One for check and another for transfer.

fixed by 57830e2

iffyio · 2024-12-13T13:28:41Z

src/parser/mod.rs

+            if dialect_of!(self is SnowflakeDialect | MsSqlDialect)
+                && self.consume_tokens(&[Token::Plus, Token::RParen])
+            {
+                Ok(Expr::OuterJoin(Box::new(
+                    match <[Ident; 1]>::try_from(id_parts) {
+                        Ok([ident]) => Expr::Identifier(ident),
+                        Err(parts) => Expr::CompoundIdentifier(parts),
+                    },
+                )))


I think it would be nice if we could reuse this logic somehow in a function? since its not trivial and lists dialects, now that its duplicated it could be easy to miss if changes are made to one but not the other?

fixed by 4b3818c

iffyio · 2024-12-13T16:13:41Z

src/parser/mod.rs

@@ -3095,15 +3201,29 @@ impl<'a> Parser<'a> {
        })
    }

+    /// Parse an multi-dimension array accessing like `[1:3][1][1]`


Suggested change

/// Parse an multi-dimension array accessing like `[1:3][1][1]`

/// Parses a multi-dimension array accessing like `[1:3][1][1]`

iffyio · 2024-12-13T16:18:04Z

src/parser/mod.rs

@@ -3095,15 +3201,29 @@ impl<'a> Parser<'a> {
        })
    }

+    /// Parse an multi-dimension array accessing like `[1:3][1][1]`
+    ///
+    /// Parser is right after the first `[`


given this is a pub function it would be nice if the api was symmetric so that the function also consumed the leading [? I think within the parser itself that would mean calling prev_token() or replacing with peek() before invoking this function which would be a fair price to pay?

Make sense.

fixed by 67cd877

iffyio

LGTM! Thanks @goldmedal!

goldmedal · 2024-12-22T14:10:06Z

Given the potential for non trivial downstream conflicts due to this change (look at the list of conflicts it has already collected) I would like to merge it in right after we have done the next release

Release sqlparser-rs version 0.53.0 / sqlparser_derive 0.3.0 #1517

@alamb @iffyio, 0.53.0 has been released. Could we merge this PR?
I want to merge it to avoid some potential conflicts.

iffyio · 2024-12-22T14:31:32Z

Thanks for working on this @goldmedal!

goldmedal · 2024-12-22T14:49:06Z

Thanks @iffyio and @alamb 👍

goldmedal added 3 commits November 25, 2024 20:46

v1 tmp

192cab4

remove MapAccess

6be3c35

fix fmt

0e916dd

goldmedal mentioned this pull request Nov 25, 2024

Enhance the nested type access for Generic and DuckDB dialect #1541

Closed

goldmedal added 12 commits November 26, 2024 00:44

remove debug message

767b531

Merge branch 'main' into feature/1533-dereference-expr-v3

22f4e67

fix span test

dee8b40

introduce CompoundExpr

fc1cd59

Merge branch 'main' into feature/1533-dereference-expr-v3

8590896

fix merge conflict

4ad37d8

replace subscript with compound expr

31a1e74

fix snowflake syntax

0355290

limit the access chain supported dialect

1de9b21

fmt

2a32b9f

enhance doc and fix the name

495d1b3

fix typo

e7b55be

goldmedal changed the title ~~Remove MapAccess and handle the field access chain~~ Introduce CompoundExpr to handle the complex field access chain Dec 4, 2024

goldmedal marked this pull request as ready for review December 4, 2024 18:04

iffyio reviewed Dec 6, 2024

View reviewed changes

alamb changed the title ~~Introduce CompoundExpr to handle the complex field access chain~~ Consolidate MapAccess, and Subscript into CompoundExpr to handle the complex field access chain Dec 6, 2024

alamb reviewed Dec 6, 2024

View reviewed changes

goldmedal added 8 commits December 9, 2024 22:45

Merge branch 'main' into feature/1533-dereference-expr-v3

6652905

update doc

47a5da1

update doc and rename AccessExpr

b58e50c

remove unused crate

7cb2e00

update the out date doc

397335a

remove unused parsing

ac25e5d

rename to CompoundFieldAccess

a08e5c2

rename chain and display AccessExpr by itself

09b39eb

goldmedal added 3 commits December 10, 2024 00:16

rename parse_compound_expr

8968fcc

fmt and clippy

1328274

fix doc

d6743e9

goldmedal mentioned this pull request Dec 11, 2024

[POC] Try to plan ast::Expr::CompoundFieldAccess syntax apache/datafusion#13734

Draft

alamb approved these changes Dec 11, 2024

View reviewed changes

iffyio reviewed Dec 13, 2024

View reviewed changes

goldmedal added 8 commits December 16, 2024 16:09

remove unnecessary check

7d030c1

improve the doc

90e03eb

remove the unused method parse_map_access

5c54d1b

avoid the unnecessary cloning

57830e2

extract parse outer_join_expr

4b3818c

consume LBarcket by parse_multi_dim_subscript

67cd877

Merge branch 'main' into feature/1533-dereference-expr-v3

23aea03

fix compile

94847d7

goldmedal requested a review from iffyio December 16, 2024 09:50

iffyio approved these changes Dec 17, 2024

View reviewed changes

iffyio merged commit 0647a4a into apache:main Dec 22, 2024
8 checks passed

goldmedal deleted the feature/1533-dereference-expr-v3 branch December 22, 2024 14:31

iffyio mentioned this pull request Dec 22, 2024

Merge composite and compound expr test cases #1615

Merged

This was referenced Dec 26, 2024

Wrong result for parsing special double number value #1619

Closed

Fix the parsing result for the special double number #1621

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate `MapAccess`, and `Subscript` into `CompoundExpr` to handle the complex field access chain #1551

Consolidate `MapAccess`, and `Subscript` into `CompoundExpr` to handle the complex field access chain #1551

goldmedal commented Nov 25, 2024 •

edited

Loading

goldmedal commented Dec 4, 2024

iffyio Dec 6, 2024

goldmedal Dec 9, 2024

iffyio Dec 13, 2024

goldmedal Dec 16, 2024

alamb left a comment

goldmedal commented Dec 9, 2024

goldmedal commented Dec 11, 2024

alamb left a comment

alamb commented Dec 11, 2024

iffyio left a comment

iffyio Dec 13, 2024

goldmedal Dec 16, 2024

goldmedal Dec 16, 2024

iffyio Dec 13, 2024

iffyio Dec 13, 2024

iffyio Dec 13, 2024

goldmedal Dec 16, 2024

goldmedal Dec 16, 2024

iffyio Dec 13, 2024

goldmedal Dec 16, 2024

iffyio Dec 13, 2024

iffyio Dec 13, 2024

goldmedal Dec 16, 2024

goldmedal Dec 16, 2024

iffyio left a comment

goldmedal commented Dec 22, 2024

iffyio commented Dec 22, 2024

goldmedal commented Dec 22, 2024

	/// Parse an multi-dimension array accessing like `[1:3][1][1]`
	/// Parses a multi-dimension array accessing like `[1:3][1][1]`

Consolidate MapAccess, and Subscript into CompoundExpr to handle the complex field access chain #1551

Consolidate MapAccess, and Subscript into CompoundExpr to handle the complex field access chain #1551

Conversation

goldmedal commented Nov 25, 2024 • edited Loading

Description

Array

Map or Struct (Bracket-style)

Struct (Period-style) (only effect when the chain contains both subscript and expr)

Breaking Change

Additional context

goldmedal commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

goldmedal commented Dec 9, 2024

goldmedal commented Dec 11, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb commented Dec 11, 2024

iffyio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

goldmedal commented Dec 22, 2024

iffyio commented Dec 22, 2024

goldmedal commented Dec 22, 2024

Consolidate `MapAccess`, and `Subscript` into `CompoundExpr` to handle the complex field access chain #1551

Consolidate `MapAccess`, and `Subscript` into `CompoundExpr` to handle the complex field access chain #1551

goldmedal commented Nov 25, 2024 •

edited

Loading

Struct (Period-style) (only effect when the chain contains both `subscript` and `expr`)