Add support for parsing MsSql alias with equals #1467

yoavcloud · 2024-10-11T06:48:56Z

This PR addresses the MsSql syntax for select item alias using the = operator, for example:

SELECT col_alias = col FROM tbl

src/parser/mod.rs

iffyio · 2024-10-13T05:17:00Z

src/parser/mod.rs

+    /// Parse a [`SelectItem`] based on an MsSql syntax that uses the equal sign
+    /// to denote an alias, for example: SELECT col_alias = col FROM tbl
+    /// <https://learn.microsoft.com/en-us/sql/t-sql/queries/select-examples-transact-sql?view=sql-server-ver16#b-use-select-with-column-headings-and-calculations>    
+    fn parse_mssql_alias_with_equal(&mut self, expr: &Expr) -> Option<SelectItem> {


Suggested change

fn parse_mssql_alias_with_equal(&mut self, expr: &Expr) -> Option<SelectItem> {

fn maybe_unpack_alias_assignment(expr: &Expr) -> Option<SelectItem> {

Thinking since this is only a helper function that it doesn't get confused for a parser/parsing method

iffyio · 2024-10-13T05:17:47Z

src/parser/mod.rs

+    /// Parse a [`SelectItem`] based on an MsSql syntax that uses the equal sign
+    /// to denote an alias, for example: SELECT col_alias = col FROM tbl
+    /// <https://learn.microsoft.com/en-us/sql/t-sql/queries/select-examples-transact-sql?view=sql-server-ver16#b-use-select-with-column-headings-and-calculations>    


Suggested change

/// Parse a [`SelectItem`] based on an MsSql syntax that uses the equal sign

/// to denote an alias, for example: SELECT col_alias = col FROM tbl

/// <https://learn.microsoft.com/en-us/sql/t-sql/queries/select-examples-transact-sql?view=sql-server-ver16#b-use-select-with-column-headings-and-calculations>

/// Parse a [`SelectItem`] based on an [MsSql] syntax that uses the equal sign

/// to denote an alias, for example: SELECT col_alias = col FROM tbl

/// [MsSql]: https://learn.microsoft.com/en-us/sql/t-sql/queries/select-examples-transact-sql?view=sql-server-ver16#b-use-select-with-column-headings-and-calculations

iffyio · 2024-10-13T05:28:11Z

src/parser/mod.rs

+        if let Expr::BinaryOp {
+            left, op, right, ..
+        } = expr
+        {
+            if op == &BinaryOperator::Eq {
+                if let Expr::Identifier(ref alias) = **left {
+                    return Some(SelectItem::ExprWithAlias {
+                        expr: *right.clone(),
+                        alias: alias.clone(),
+                    });
+                }
+            }
+        }
+
+        None


Suggested change

if let Expr::BinaryOp {

left, op, right, ..

} = expr

{

if op == &BinaryOperator::Eq {

if let Expr::Identifier(ref alias) = **left {

return Some(SelectItem::ExprWithAlias {

expr: *right.clone(),

alias: alias.clone(),

});

}

}

}

None

if let Expr::BinaryOp {

left: Expr::Identifier(alias), op: BinaryOperator::Eq, right,

} = expr {

SelectItem::ExprWithAlias {

expr: right,

alias: Expr::Identifier(alias),

}

} else {

expr

}

Could be simplified to the following? I think no need to pass in a reference and return an option, we can move the value and return the same value if untouched?
Also we can skip the .. operator when deconstructing in this case, so that if the field set changes, we would be notified to ensure the logic remains correct.

But this would make the function return two different types, as SelectItem is not an Expr variant, no?

Ah yeah that's correct, actually I'm thinking in that case we can inline the function since its small enough?

yoavcloud · 2024-10-14T06:06:56Z

@iffyio I tried inlining the function, it's not as short as I wish it would, due to pattern matching the boxed fields. But maybe I'm missing something... Would love your feedback.

iffyio · 2024-10-14T07:10:02Z

src/dialect/mod.rs

@@ -561,6 +561,11 @@ pub trait Dialect: Debug + Any {
    fn supports_asc_desc_in_column_definition(&self) -> bool {
        false
    }
+
+    /// For example: SELECT col_alias = col FROM tbl


Suggested change

/// For example: SELECT col_alias = col FROM tbl

/// Returns true if this dialect supports treating the equals operator `=` within a [`SelectItem`]

/// as an alias assignment operator, rather than a boolean expression.

/// For example: the following statements are equivalent for such a dialect:

/// ```sql

/// SELECT col_alias = col FROM tbl;

/// SELECT col_alias AS col FROM tbl;

/// ```

Something like this to illustrate what the flag implies Im thinking?

iffyio · 2024-10-14T07:13:51Z

src/parser/mod.rs

+                // Parse a [`SelectItem`] based on an [MsSql] syntax that uses the equal sign
+                // to denote an alias, for example: SELECT col_alias = col FROM tbl
+                // [MsSql]: https://learn.microsoft.com/en-us/sql/t-sql/queries/select-examples-transact-sql?view=sql-server-ver16#b-use-select-with-column-headings-and-calculations


Suggested change

// Parse a [`SelectItem`] based on an [MsSql] syntax that uses the equal sign

// to denote an alias, for example: SELECT col_alias = col FROM tbl

// [MsSql]: https://learn.microsoft.com/en-us/sql/t-sql/queries/select-examples-transact-sql?view=sql-server-ver16#b-use-select-with-column-headings-and-calculations

We can add the mssql link as part of the trait impl for the dialect instead? Then here I think we would be able to skip the comment altogether since the self.dialect.supports_eq_alias_assignment() along with the documentation on the trait definition for the method would suffice. It'll let us keep the doc for the feature centralized

iffyio · 2024-10-14T07:26:21Z

src/parser/mod.rs

+                let expr = if self.dialect.supports_eq_alias_assigment() {
+                    if let Expr::BinaryOp {
+                        ref left,
+                        op: BinaryOperator::Eq,
+                        ref right,
+                    } = expr
+                    {
+                        if let Expr::Identifier(alias) = left.as_ref() {
+                            return Ok(SelectItem::ExprWithAlias {
+                                expr: *right.clone(),
+                                alias: alias.clone(),
+                            });
+                        }
+                    }
+                    expr
+                } else {
+                    expr
+                };
+
+                // Parse the common AS keyword for aliasing a column
+                self.parse_optional_alias(keywords::RESERVED_FOR_COLUMN_ALIAS)
+                    .map(|alias| match alias {
+                        Some(alias) => SelectItem::ExprWithAlias { expr, alias },
+                        None => SelectItem::UnnamedExpr(expr),
+                    })


Suggested change

let expr = if self.dialect.supports_eq_alias_assigment() {

if let Expr::BinaryOp {

ref left,

op: BinaryOperator::Eq,

ref right,

} = expr

{

if let Expr::Identifier(alias) = left.as_ref() {

return Ok(SelectItem::ExprWithAlias {

expr: *right.clone(),

alias: alias.clone(),

});

}

}

expr

} else {

expr

};

// Parse the common AS keyword for aliasing a column

self.parse_optional_alias(keywords::RESERVED_FOR_COLUMN_ALIAS)

.map(|alias| match alias {

Some(alias) => SelectItem::ExprWithAlias { expr, alias },

None => SelectItem::UnnamedExpr(expr),

})

let (expr, alias) = match expr {

Expr::BinaryOp {

left: Expr::Identifier(alias),

BinaryOperator::Eq,

right,

} if self.dialect.supports_eq_alias_assigment() => {

(expr, Some(alias))

}

expr => {

// Parse the common AS keyword for aliasing a column

self.parse_optional_alias(keywords::RESERVED_FOR_COLUMN_ALIAS)?

}

};

Ok(alias

.map(|alias| SelectItem::ExprWithAlias{ expr, alias })

.unwrap_or_else(|| SelectItem::UnnamedExpr(expr)))

The current inline looks reasonable, I think its mostly rustfmt that making it look a like it has bit more code than it does, but I think we could simplify the logic a bit as above to help with that and readability/intent as well? hopefully?

I don't think you can match to Expr::Identifier in L11182 because left is a boxed field. This further complicates the logic of the function because you now need another if let condition to check the value of left. The complexity ends up similar to what I suggested.

I suggest going back to the helper function design, it cleanly encapsulates this rather edgy case of MsSql without complicating the main flow of parsing the common syntax for aliasing a column.

this works as its own match clause for example:

Expr::BinaryOp { left, op: BinaryOperator::Eq, right, } if self.dialect.supports_eq_alias_assigment() && matches!(left.as_ref(), Expr::Identifier(_)) => { let Expr::Identifier(alias) = *left else { return parser_err!( "BUG: expected identifier expression as alias", self.peek_token().location ); }; Ok(SelectItem::ExprWithAlias { expr: *right, alias, }) }

I think the least desirable option involves the expression cloning, i.e. the right.clone() I think we want to avoid in any case

Impressive! Adopted in the next commit

iffyio · 2024-10-14T07:33:26Z

tests/sqlparser_mssql.rs

@@ -1024,6 +1024,17 @@ fn parse_create_table_with_identity_column() {
    }
 }

+#[test]


Ah sorry I forgot to mention this part earlier, the test looks reasonable but now with the method on the dialect, can we move the test to common.rs and write it in a similar style to this? that would allow any future dialects with the same support to be covered automatically by the test.

Also for this feature, can we include an assertion that for the dialects that don't support the flag, verifying that they indeed end up with an unnamed select item?

yoavcloud · 2024-10-14T20:25:19Z

@iffyio Fixed all comments but went back to a helper function design as the elegance of the parse_select_item function is compromised by inlining this logic.

coveralls · 2024-10-16T16:30:26Z

Pull Request Test Coverage Report for Build 11334438657

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

27 of 30 (90.0%) changed or added relevant lines in 4 files are covered.
366 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.007%) to 89.323%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/dialect/mod.rs	1	2	50.0%
src/dialect/mssql.rs	1	2	50.0%
tests/sqlparser_common.rs	4	5	80.0%

Files with Coverage Reduction	New Missed Lines	%
src/parser/mod.rs	105	93.4%
tests/sqlparser_common.rs	261	89.59%

Totals
Change from base Build 11294882452:	0.007%
Covered Lines:	30193
Relevant Lines:	33802

💛 - Coveralls

yoavcloud · 2024-10-16T20:14:08Z

@iffyio I adopted your change to inline the function in the latest commit

iffyio · 2024-10-17T16:00:40Z

tests/sqlparser_common.rs

+
+#[test]
+fn test_alias_equal_expr() {
+    let dialects = all_dialects_where(|d| d.supports_eq_alias_assigment());


@yoavcloud just one comment on the test from here if we can include a similar case for the other dialects verifying that the some_alias = some_column gets parsed as an actual expression vs an alias?

Beyond that I think this should be good to go, thanks!

Good idea, done!

…y equals sign

iffyio

LGTM! cc @alamb

alamb

Thank you @yoavcloud and @iffyio

iffyio reviewed Oct 13, 2024

View reviewed changes

yoavcloud force-pushed the mssql_alias_with_equals branch from 383226c to cb7eeed Compare October 14, 2024 06:03

yoavcloud requested a review from iffyio October 14, 2024 06:07

iffyio reviewed Oct 14, 2024

View reviewed changes

yoavcloud requested a review from iffyio October 14, 2024 20:25

yoavcloud added 4 commits October 16, 2024 22:10

Add support for parsing MsSql alias with equals

2ee78ef

Try to inline the function based on code review feedback

1fedea7

Go back to helper function

79a91d8

Inline the function wihtout cloning

e9de85d

yoavcloud force-pushed the mssql_alias_with_equals branch from 92ea0f4 to e9de85d Compare October 16, 2024 20:12

iffyio reviewed Oct 17, 2024

View reviewed changes

Add test case for dialects that do not support the alias assignment b…

90960e9

…y equals sign

yoavcloud requested a review from iffyio October 17, 2024 16:50

iffyio approved these changes Oct 17, 2024

View reviewed changes

alamb approved these changes Oct 20, 2024

View reviewed changes

Fix docs

5afda20

alamb merged commit 1dd7d26 into apache:main Oct 20, 2024
10 checks passed

alamb mentioned this pull request Oct 21, 2024

Fix typo in Dialect::supports_eq_alias_assigment #1478

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for parsing MsSql alias with equals #1467

Add support for parsing MsSql alias with equals #1467

yoavcloud commented Oct 11, 2024

iffyio Oct 13, 2024

iffyio Oct 13, 2024

iffyio Oct 13, 2024

yoavcloud Oct 13, 2024

iffyio Oct 13, 2024

yoavcloud commented Oct 14, 2024

iffyio Oct 14, 2024

iffyio Oct 14, 2024

iffyio Oct 14, 2024

yoavcloud Oct 14, 2024

iffyio Oct 15, 2024

yoavcloud Oct 16, 2024

iffyio Oct 14, 2024

yoavcloud commented Oct 14, 2024

coveralls commented Oct 16, 2024

yoavcloud commented Oct 16, 2024

iffyio Oct 17, 2024

yoavcloud Oct 17, 2024

iffyio left a comment

alamb left a comment

	fn parse_mssql_alias_with_equal(&mut self, expr: &Expr) -> Option<SelectItem> {
	fn maybe_unpack_alias_assignment(expr: &Expr) -> Option<SelectItem> {

-    /// For example: SELECT col_alias = col FROM tbl
+    /// Returns true if this dialect supports treating the equals operator `=` within a [`SelectItem`]
+    /// as an alias assignment operator, rather than a boolean expression.
+    /// For example: the following statements are equivalent for such a dialect:
+    /// ```sql
+    ///  SELECT col_alias = col FROM tbl;
+    ///  SELECT col_alias AS col FROM tbl;
+    /// ```

	// Parse a [`SelectItem`] based on an [MsSql] syntax that uses the equal sign
	// to denote an alias, for example: SELECT col_alias = col FROM tbl
	// [MsSql]: https://learn.microsoft.com/en-us/sql/t-sql/queries/select-examples-transact-sql?view=sql-server-ver16#b-use-select-with-column-headings-and-calculations

@@ @@ -1024,6 +1024,17 @@ fn parse_create_table_with_identity_column() { @@
                   }
               }
+              #[test]

Add support for parsing MsSql alias with equals #1467

Add support for parsing MsSql alias with equals #1467

Conversation

yoavcloud commented Oct 11, 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

Choose a reason for hiding this comment

yoavcloud commented Oct 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoavcloud commented Oct 14, 2024

coveralls commented Oct 16, 2024

Pull Request Test Coverage Report for Build 11334438657

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

yoavcloud commented Oct 16, 2024

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

alamb left a comment

Choose a reason for hiding this comment