-
Notifications
You must be signed in to change notification settings - Fork 756
Add xmlnamespace support mssql #2361
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
bb95612
5d63c97
a903a0c
2dbfcba
7d0b184
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -248,6 +248,11 @@ impl Dialect for MsSqlDialect { | |
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| // see: https://learn.microsoft.com/en-us/sql/t-sql/xml/with-xmlnamespaces | ||
| fn supports_with_xmlnamespaces_clause(&self) -> bool { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we include a link to the documentation of this syntax?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iffyio Added documentation link |
||
| true | ||
| } | ||
| } | ||
|
|
||
| impl MsSqlDialect { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2923,3 +2923,10 @@ fn parse_mssql_money_constants() { | |
| expr_from_projection(only(&select.projection)), | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn parse_xmlnamespaces() { | ||
|
|
||
| ms().verified_stmt("WITH XMLNAMESPACES ('urn:test' AS ns) SELECT 1 AS [ns:Value] FOR XML PATH('ns:Root')"); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. merged tests and used verifies_stmt() @iffyio |
||
| ms().verified_stmt("WITH XMLNAMESPACES ('urn:example' AS ns), t AS (SELECT 1 AS id) SELECT id FROM t"); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| /// Test to verify XMLNAMESPACES parsing and AST storage | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm I don't think we should introduce a new file for this test. Also let's merge the tests and used either verified_stmt or one_statement_parses_to in tests as I mentioned in the previous review. one simplification of the tests introduced here is to drop the AST assertions, the PR doesn't introduce a new node so its overkill to have each test expiclitly assert the full AST. please have the tests follow existing conventions (this is introducing some println and manual display assertions patterns and is unclear why that's needed)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you are correct, i removed the file. |
||
| /// This demonstrates that the XMLNAMESPACES clause is now properly stored in the AST | ||
| /// instead of being dropped. | ||
| use sqlparser::ast::Statement; | ||
| use sqlparser::dialect::MsSqlDialect; | ||
| use sqlparser::parser::Parser; | ||
|
|
||
| #[test] | ||
| fn test_xmlnamespaces_parsing_and_ast_storage() { | ||
| let dialect = MsSqlDialect {}; | ||
| let sql = r#" | ||
| WITH XMLNAMESPACES ('http://example.com' AS ex, 'http://other.com' AS ot) | ||
| SELECT 1 AS col | ||
| "#; | ||
|
|
||
| let mut parser = Parser::new(&dialect).try_with_sql(sql).unwrap(); | ||
| let ast = parser.parse_statements().unwrap(); | ||
|
|
||
| assert_eq!(ast.len(), 1, "Should parse as a single statement"); | ||
|
|
||
| match &ast[0] { | ||
| Statement::Query(query) => { | ||
| // Verify the WITH clause is present | ||
| assert!(query.with.is_some(), "Query should have WITH clause"); | ||
|
|
||
| let with_clause = query.with.as_ref().unwrap(); | ||
|
|
||
| // Verify xml_namespaces were captured | ||
| assert_eq!( | ||
| with_clause.xml_namespaces.len(), | ||
| 2, | ||
| "Should have 2 XML namespace definitions" | ||
| ); | ||
|
|
||
| // Check first namespace | ||
| let first_ns = &with_clause.xml_namespaces[0]; | ||
| assert_eq!( | ||
| first_ns.name.value, "ex", | ||
| "First namespace alias should be 'ex'" | ||
| ); | ||
|
|
||
| // Check second namespace | ||
| let second_ns = &with_clause.xml_namespaces[1]; | ||
| assert_eq!( | ||
| second_ns.name.value, "ot", | ||
| "Second namespace alias should be 'ot'" | ||
| ); | ||
|
|
||
| // Verify CTEs are empty (no CTEs after XMLNAMESPACES in this example) | ||
| assert_eq!(with_clause.cte_tables.len(), 0, "Should have no CTE tables"); | ||
|
|
||
| // Verify Display output includes XMLNAMESPACES | ||
| let display_output = format!("{}", with_clause); | ||
| assert!( | ||
| display_output.contains("XMLNAMESPACES"), | ||
| "Display output should include XMLNAMESPACES" | ||
| ); | ||
|
|
||
| println!("✓ XMLNAMESPACES AST representation: {}", display_output); | ||
| } | ||
| _ => panic!("Expected Query statement"), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_xmlnamespaces_with_ctes() { | ||
| let dialect = MsSqlDialect {}; | ||
| let sql = r#" | ||
| WITH XMLNAMESPACES ('http://example.com' AS ex), | ||
| cte1 AS (SELECT 1 AS col) | ||
| SELECT * FROM cte1 | ||
| "#; | ||
|
|
||
| let mut parser = Parser::new(&dialect).try_with_sql(sql).unwrap(); | ||
| let ast = parser.parse_statements().unwrap(); | ||
|
|
||
| assert_eq!(ast.len(), 1, "Should parse as a single statement"); | ||
|
|
||
| match &ast[0] { | ||
| Statement::Query(query) => { | ||
| let with_clause = query.with.as_ref().unwrap(); | ||
|
|
||
| // Verify namespaces | ||
| assert_eq!( | ||
| with_clause.xml_namespaces.len(), | ||
| 1, | ||
| "Should have 1 XML namespace definition" | ||
| ); | ||
|
|
||
| // Verify CTEs | ||
| assert_eq!(with_clause.cte_tables.len(), 1, "Should have 1 CTE table"); | ||
| assert_eq!( | ||
| with_clause.cte_tables[0].alias.name.value, "cte1", | ||
| "CTE name should be 'cte1'" | ||
| ); | ||
|
|
||
| let display_output = format!("{}", with_clause); | ||
| println!("✓ XMLNAMESPACES with CTEs: {}", display_output); | ||
| assert!(display_output.contains("XMLNAMESPACES")); | ||
| assert!(display_output.contains("cte1")); | ||
| } | ||
| _ => panic!("Expected Query statement"), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_xmlnamespaces_display_format() { | ||
| let dialect = MsSqlDialect {}; | ||
| let sql = r#" | ||
| WITH XMLNAMESPACES ('http://example.com' AS ex, 'http://other.com' AS ot), | ||
| my_cte AS (SELECT 1) | ||
| SELECT * FROM my_cte | ||
| "#; | ||
|
|
||
| let mut parser = Parser::new(&dialect).try_with_sql(sql).unwrap(); | ||
| let ast = parser.parse_statements().unwrap(); | ||
|
|
||
| match &ast[0] { | ||
| Statement::Query(query) => { | ||
| let with_clause = query.with.as_ref().unwrap(); | ||
| let display_output = format!("{}", with_clause); | ||
|
|
||
| // Verify the order: XMLNAMESPACES comes first, then CTEs | ||
| assert!( | ||
| display_output.starts_with("WITH XMLNAMESPACES"), | ||
| "Display should start with 'WITH XMLNAMESPACES'" | ||
| ); | ||
|
|
||
| println!("✓ Full display format: {}", display_output); | ||
| } | ||
| _ => panic!("Expected Query statement"), | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feature looks similar to what was done here for clickhouse CSEs, such that I'm thinking we essentially want to introduce this feature in that style instead. is xml_namespaces looks like a regular expression (CSE) so that the enum changes might even be verbatim (similarly for the dialect method name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed i use the WITH keyword as done with cte_tables, i hope i understood what you have meant.