Add xmlnamespace support mssql - #2361
Conversation
|
Hi @iffyio CAn you please help with this PR as well? |
| fn parse_xmlnamespaces() { | ||
| let sql = r#"WITH XMLNAMESPACES ('urn:test' AS ns) | ||
| SELECT 1 AS [ns:Value] | ||
| FOR XML PATH('ns:Root');"#; | ||
|
|
||
| ms().parse_sql_statements(sql).unwrap(); | ||
|
|
||
| } | ||
|
|
||
| #[test] | ||
| fn parse_xmlnamespaces_with_cte() { | ||
| let sql = r#" | ||
| WITH XMLNAMESPACES ('urn:example' AS ns), t AS ( | ||
| SELECT 1 AS id | ||
| ) | ||
| SELECT id FROM t | ||
| "#; | ||
|
|
||
| ms().parse_sql_statements(sql).unwrap(); | ||
| } No newline at end of file |
There was a problem hiding this comment.
let's merge these and have them use verified_stmt
| let _namespaces = | ||
| self.parse_comma_separated(Parser::parse_xml_namespace_definition)?; | ||
| self.expect_token(&Token::RParen)?; | ||
|
|
||
| if self.consume_token(&Token::Comma) { | ||
| Some(With { | ||
| with_token: with_token.clone().into(), | ||
| recursive: self.parse_keyword(Keyword::RECURSIVE), | ||
| cte_tables: self.parse_comma_separated(Parser::parse_cte)?, | ||
| }) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
the impl looks odd, is it parsing and dropping the content?
There was a problem hiding this comment.
You are correct, I missed the addition to AST fixed now
| } | ||
| } | ||
|
|
||
| fn supports_with_xmlnamespaces_clause(&self) -> bool { |
There was a problem hiding this comment.
can we include a link to the documentation of this syntax?
|
|
||
| ms().parse_sql_statements(sql).unwrap(); | ||
|
|
||
| ms().verified_stmt("WITH XMLNAMESPACES ('urn:test' AS ns) SELECT 1 AS [ns:Value] FOR XML PATH('ns:Root')"); |
| @@ -0,0 +1,133 @@ | |||
| /// Test to verify XMLNAMESPACES parsing and AST storage | |||
There was a problem hiding this comment.
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)
| /// The list of CTEs declared by this `WITH` clause. | ||
| pub cte_tables: Vec<Cte>, | ||
| /// Optional XML namespace definitions (`WITH XMLNAMESPACES (...)`). | ||
| pub xml_namespaces: Vec<XmlNamespaceDefinition>, |
There was a problem hiding this comment.
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.
Indeed i use the WITH keyword as done with cte_tables, i hope i understood what you have meant.
2 participants
Added support to parse xmlnamespace in mssql, for queris like:
WITH XMLNAMESPACES ('urn:test' AS ns) SELECT 1 AS [ns:Value] FOR XML PATH('ns:Root');