iffyio commented on code in PR #2361:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2361#discussion_r3681327534
##########
tests/test_xmlnamespace_integration.rs:
##########
@@ -0,0 +1,133 @@
+/// Test to verify XMLNAMESPACES parsing and AST storage
Review 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)
##########
src/ast/query.rs:
##########
@@ -756,11 +756,23 @@ pub struct With {
pub recursive: bool,
/// 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>,
Review Comment:
this feature looks similar to what was [done
here](https://github.com/apache/datafusion-sqlparser-rs/pull/2333/changes) 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
