Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for MSSQL's XQuery methods #1500

Merged
merged 21 commits into from
Nov 14, 2024

Conversation

gaoqiangz
Copy link
Contributor

Supports XQuery methods:

SELECT STUFF((SELECT ',' + name FROM sys.objects FOR XML PATH(''), TYPE).value('.', 'NVARCHAR(MAX)'), 1, 1, '') AS T
SELECT CAST(column AS XML).value('.', 'NVARCHAR(MAX)') AS T

See https://learn.microsoft.com/en-us/sql/t-sql/xml/xml-data-type-methods?view=sql-server-ver16.

src/ast/mod.rs Outdated Show resolved Hide resolved
src/parser/mod.rs Outdated Show resolved Hide resolved
@gaoqiangz
Copy link
Contributor Author

I redesigned CompositeFunction, please take a look.

tests/sqlparser_common.rs Outdated Show resolved Hide resolved
src/ast/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a quick look and left some highlevel comment, will make time soon to take a closer look. Thanks for clarifying the behavior @gaoqiangz together with the rework the intention is much clearer to me!

src/parser/mod.rs Outdated Show resolved Hide resolved
src/parser/mod.rs Outdated Show resolved Hide resolved
@gaoqiangz
Copy link
Contributor Author

I added supports_methods to the Dialect trait and try_parse_method to the Parser, this might be better suited for different scenarios and doesn't break existing behavior.

Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaoqiangz took a look now and left some comments

src/parser/mod.rs Outdated Show resolved Hide resolved
src/ast/mod.rs Outdated Show resolved Hide resolved
src/ast/mod.rs Show resolved Hide resolved
src/ast/mod.rs Show resolved Hide resolved
src/ast/mod.rs Outdated Show resolved Hide resolved
src/parser/mod.rs Outdated Show resolved Hide resolved
src/parser/mod.rs Outdated Show resolved Hide resolved
@gaoqiangz
Copy link
Contributor Author

The code has been rewritten for the new Method definition, seems more reasonable:

pub struct Method {
    pub expr: Box<Expr>,
    // always non-empty
    pub method_chain: Vec<Function>,
}

Any other comments?

src/ast/mod.rs Show resolved Hide resolved
src/parser/mod.rs Show resolved Hide resolved
src/parser/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! cc @alamb

src/ast/mod.rs Show resolved Hide resolved
@iffyio
Copy link
Contributor

iffyio commented Nov 9, 2024

@gaoqiangz could you take a look at the doc building failure when you get the chance?

@gaoqiangz
Copy link
Contributor Author

@iffyio CI was fixed.

@alamb
Copy link
Contributor

alamb commented Nov 14, 2024

🚀

@alamb alamb merged commit 62eaee6 into apache:main Nov 14, 2024
8 checks passed
@alamb
Copy link
Contributor

alamb commented Nov 14, 2024

Thanks again @gaoqiangz and @lovasoa and @iffyio

@gaoqiangz gaoqiangz deleted the support_mssql_xquery branch November 17, 2024 12:37
@yoavcloud
Copy link
Contributor

@gaoqiangz I've been working on a similar addition, it's great to see you beat me to it :-) One thing I added in my (unpublished) work is data-type specific methods. For example:
SELECT geography::STGeomFromText('POLYGON((-122.358 47.653, -122.348 47.649, -122.348 47.658, -122.358 47.658, -122.358 47.653))', 4326).STAsText() FROM customers

How would you support the geography data type here? In my code I added it as a namespace field. I'm trying to avoid expanding ObjectName. Do you think that can work in your code too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants