iffyio commented on code in PR #1891:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1891#discussion_r2166403214
##
tests/sqlparser_snowflake.rs:
##
@@ -4165,3 +4167,10 @@ fn test_snowflake_fetch_clause_syntax() {
canonical,
);
}
+
+#[test]
+fn test_snowflake_create_view_with_tag() {
+let create_view_with_tag =
+r#"CREATE VIEW X (COL WITH TAG (pii='email') COMMENT 'foobar') AS
SELECT * FROM Y"#;
Review Comment:
is the intent to verify the behavior of tag or multiple comments support? my
understanding is the latter so that we could probably rename the test to match?
##
tests/sqlparser_common.rs:
##
@@ -7964,7 +7964,7 @@ fn parse_create_view_with_columns() {
let sql = "CREATE VIEW v (has, cols) AS SELECT 1, 2";
// TODO: why does this fail for ClickHouseDialect? (#1449)
// match all_dialects().verified_stmt(sql) {
-match all_dialects_except(|d|
d.is::()).verified_stmt(sql) {
+match all_dialects_where(|d|
!d.is::()).verified_stmt(sql) {
Review Comment:
Was there a requirement to change this? on a glance it doesn't look like any
functionality is changed and if so we can likely revert this diff
##
src/ast/ddl.rs:
##
@@ -1436,7 +1453,11 @@ impl fmt::Display for ViewColumnDef {
write!(f, " {}", data_type)?;
}
if let Some(options) = self.options.as_ref() {
-write!(f, " {}", display_comma_separated(options.as_slice()))?;
+if matches!(options, ColumnOptions::CommaSeparated(_)) {
+write!(f, " {}", display_comma_separated(options.as_slice()))?;
+} else {
+write!(f, " {}", display_separated(options.as_slice(), " "))?
+}
Review Comment:
Can we make this an exhaustive match? that way if new variants are added the
compiler enforces that this code is revisited. e.g.
```rust
match options {
ColumnOptions::CommaSeparated(_) => {}
ColumnOptions::SpaceSeparated(_) => {}
}
```
##
src/ast/spans.rs:
##
@@ -992,8 +992,11 @@ impl Spanned for ViewColumnDef {
} = self;
union_spans(
-core::iter::once(name.span)
-.chain(options.iter().flat_map(|i| i.iter().map(|k|
k.span(,
+core::iter::once(name.span).chain(
+options
+.iter()
+.flat_map(|i| i.as_slice().iter().map(|k| k.span())),
Review Comment:
Can we `impl Spanned for ColumnOptions` instead? Then here we'll call the
`span()` function, and we can drop the `as_slice()` custom function for the
struct
##
src/parser/mod.rs:
##
@@ -10602,6 +10593,26 @@ impl<'a> Parser<'a> {
})
}
+fn parse_view_column_options(&mut self) -> Result,
ParserError> {
+let mut options = Vec::new();
+loop {
+let option = self.parse_optional_column_option()?;
+if let Some(option) = option {
+options.push(option);
+} else {
+break;
+}
+}
+Ok(options
+.is_empty()
+.not()
+.then_some(if dialect_of!(self is SnowflakeDialect) {
+ColumnOptions::SpaceSeparated(options)
+} else {
+ColumnOptions::CommaSeparated(options)
+}))
Review Comment:
Can we write this to not use the `not()` method? I think it currently
obfuscates what's going on. Instead we can use an if/else `if
!options.is_empty() {} else {}`
##
src/parser/mod.rs:
##
@@ -10602,6 +10593,26 @@ impl<'a> Parser<'a> {
})
}
+fn parse_view_column_options(&mut self) -> Result,
ParserError> {
+let mut options = Vec::new();
+loop {
+let option = self.parse_optional_column_option()?;
+if let Some(option) = option {
+options.push(option);
+} else {
+break;
+}
+}
+Ok(options
+.is_empty()
+.not()
+.then_some(if dialect_of!(self is SnowflakeDialect) {
Review Comment:
Instead of using the `dialect_of` macro, we can add a `if
self.dialect.supports_space_separated_column_options()` method. The macro is
effectively deprecated and we try to avoid it in new code
--
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]
