LucaCappelletti94 commented on code in PR #2489:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2489#discussion_r4103488147
##########
src/ast/data_type.rs:
##########
@@ -922,6 +936,11 @@ pub enum StructBracketKind {
pub enum MapBracketKind {
/// Example: `Map(String, UInt16)`
Parentheses,
+ /// Example: `MAP(VARCHAR, NUMBER NOT NULL)`
+ SnowflakeParentheses {
+ /// Whether map values must be non-null.
+ value_not_null: bool,
+ },
Review Comment:
You should rename `SnowflakeParentheses { value_not_null }` to a unit
variant `ParenthesesNotNull`, matching `ArrayElemTypeDef::ParenthesisNotNull`.
```suggestion
/// Example: `MAP(VARCHAR, NUMBER NOT NULL)`
ParenthesesNotNull,
```
##########
src/ast/data_type.rs:
##########
@@ -795,6 +799,13 @@ impl fmt::Display for DataType {
MapBracketKind::Parentheses => {
write!(f, "Map({key_data_type}, {value_data_type})")
}
+ MapBracketKind::SnowflakeParentheses { value_not_null } => {
+ write!(f, "MAP({key_data_type}, {value_data_type}")?;
+ if *value_not_null {
+ write!(f, " NOT NULL")?;
+ }
+ write!(f, ")")
+ }
Review Comment:
You should render the new variant the way
`ArrayElemTypeDef::ParenthesisNotNull` renders.
```suggestion
MapBracketKind::ParenthesesNotNull => {
write!(f, "MAP({key_data_type}, {value_data_type} NOT
NULL)")
}
```
##########
tests/sqlparser_snowflake.rs:
##########
@@ -4954,6 +4954,85 @@ fn test_structured_array_type() {
);
}
+#[test]
+fn test_structured_object_type() {
+ snowflake_and_generic().verified_stmt(
+ "SELECT payload::OBJECT(address OBJECT(city VARCHAR NOT NULL), zip
NUMBER) FROM t",
+ );
+ snowflake()
+ .verified_stmt("SELECT payload::OBJECT(tags ARRAY, labels MAP(VARCHAR,
VARCHAR)) FROM t");
Review Comment:
You should expect `Map(...)` here, the same way `ARRAY(NUMBER)` already
renders as `Array(NUMBER)` in this test, otherwise without the casing
normalization ClickHouse would stop parsing.
```suggestion
snowflake().one_statement_parses_to(
"SELECT payload::OBJECT(tags ARRAY, labels MAP(VARCHAR, VARCHAR))
FROM t",
"SELECT payload::OBJECT(tags ARRAY, labels Map(VARCHAR, VARCHAR))
FROM t",
);
```
##########
src/parser/mod.rs:
##########
@@ -13216,13 +13241,20 @@ impl<'a> Parser<'a> {
MapBracketKind::AngleBrackets,
))
}
- Keyword::MAP if dialect_is!(dialect is ClickHouseDialect |
GenericDialect) => {
+ Keyword::MAP if dialect_is!(dialect is ClickHouseDialect |
GenericDialect | SnowflakeDialect) =>
Review Comment:
You should replace the widened `dialect_is!` list with a dialect method, as
#2488 did for `ARRAY(...)`, and derive the bracket kind from the parsed `NOT
NULL` so the AST no longer depends on the dialect. The diffs here is complex so
I cannot give you a suggestion to immediately apply, but I imagine it is a
rather trivial change to make.
##########
tests/sqlparser_snowflake.rs:
##########
@@ -4954,6 +4954,85 @@ fn test_structured_array_type() {
);
}
+#[test]
+fn test_structured_object_type() {
+ snowflake_and_generic().verified_stmt(
+ "SELECT payload::OBJECT(address OBJECT(city VARCHAR NOT NULL), zip
NUMBER) FROM t",
+ );
+ snowflake()
+ .verified_stmt("SELECT payload::OBJECT(tags ARRAY, labels MAP(VARCHAR,
VARCHAR)) FROM t");
+ let select = snowflake().verified_only_select(
+ "SELECT payload::OBJECT(items ARRAY(NUMBER NOT NULL), meta
MAP(VARCHAR, OBJECT(k NUMBER) NOT NULL)) FROM t",
+ );
+ let Expr::Cast { data_type, .. } =
expr_from_projection(only(&select.projection)) else {
+ unreachable!();
+ };
+ let DataType::Object(fields) = data_type else {
+ unreachable!();
+ };
+ assert!(matches!(
+ &fields[0].data_type,
+ DataType::Array(ArrayElemTypeDef::ParenthesisNotNull(_))
+ ));
+ let DataType::Map(
+ _,
+ value,
+ MapBracketKind::SnowflakeParentheses {
+ value_not_null: true,
+ },
+ ) = &fields[1].data_type
+ else {
Review Comment:
You should match the renamed variant.
```suggestion
let DataType::Map(_, value, MapBracketKind::ParenthesesNotNull) =
&fields[1].data_type else {
```
##########
src/parser/mod.rs:
##########
@@ -13178,6 +13182,27 @@ impl<'a> Parser<'a> {
))))
}
}
+ Keyword::OBJECT
+ if self.peek_token_ref().token == Token::LParen
+ && (dialect_is!(dialect is SnowflakeDialect)
+ || !matches!(
+ self.peek_nth_token_ref(1).token,
+ Token::SingleQuotedString(_)
+ )) =>
+ {
+ if dialect_is!(dialect is SnowflakeDialect) {
+
Ok(DataType::Object(self.parse_structured_object_type_def()?))
+ } else if let Some(fields) =
+ self.maybe_parse(|parser|
parser.parse_structured_object_type_def())?
+ {
+ Ok(DataType::Object(fields))
+ } else {
+ self.prev_token();
+ let type_name = self.parse_object_name(false)?;
+ let modifiers =
self.parse_optional_type_modifiers()?.unwrap_or_default();
+ Ok(DataType::Custom(type_name, modifiers))
+ }
+ }
Review Comment:
You should drop the `SnowflakeDialect` checks and give every dialect the
same structured-then-custom path.
```suggestion
Keyword::OBJECT if self.peek_token_ref().token ==
Token::LParen => {
if let Some(fields) =
self.maybe_parse(|parser|
parser.parse_structured_object_type_def())?
{
Ok(DataType::Object(fields))
} else {
self.prev_token();
let type_name = self.parse_object_name(false)?;
let modifiers =
self.parse_optional_type_modifiers()?.unwrap_or_default();
Ok(DataType::Custom(type_name, modifiers))
}
}
```
##########
tests/sqlparser_snowflake.rs:
##########
@@ -4954,6 +4954,85 @@ fn test_structured_array_type() {
);
}
+#[test]
+fn test_structured_object_type() {
+ snowflake_and_generic().verified_stmt(
+ "SELECT payload::OBJECT(address OBJECT(city VARCHAR NOT NULL), zip
NUMBER) FROM t",
+ );
+ snowflake()
+ .verified_stmt("SELECT payload::OBJECT(tags ARRAY, labels MAP(VARCHAR,
VARCHAR)) FROM t");
+ let select = snowflake().verified_only_select(
+ "SELECT payload::OBJECT(items ARRAY(NUMBER NOT NULL), meta
MAP(VARCHAR, OBJECT(k NUMBER) NOT NULL)) FROM t",
+ );
+ let Expr::Cast { data_type, .. } =
expr_from_projection(only(&select.projection)) else {
+ unreachable!();
+ };
+ let DataType::Object(fields) = data_type else {
+ unreachable!();
+ };
+ assert!(matches!(
+ &fields[0].data_type,
+ DataType::Array(ArrayElemTypeDef::ParenthesisNotNull(_))
+ ));
+ let DataType::Map(
+ _,
+ value,
+ MapBracketKind::SnowflakeParentheses {
+ value_not_null: true,
+ },
+ ) = &fields[1].data_type
+ else {
+ unreachable!();
+ };
+ assert!(matches!(**value, DataType::Object(_)));
+
+ snowflake().one_statement_parses_to(
+ "SELECT payload::ARRAY(NUMBER) FROM t",
+ "SELECT payload::Array(NUMBER) FROM t",
+ );
+ snowflake().verified_stmt("SELECT payload::MAP(VARCHAR, OBJECT(k NUMBER))
FROM t");
Review Comment:
You should expect `Map(...)` here as well.
```suggestion
snowflake().one_statement_parses_to(
"SELECT payload::MAP(VARCHAR, OBJECT(k NUMBER)) FROM t",
"SELECT payload::Map(VARCHAR, OBJECT(k NUMBER)) FROM t",
);
```
##########
tests/sqlparser_snowflake.rs:
##########
@@ -4954,6 +4954,85 @@ fn test_structured_array_type() {
);
}
+#[test]
+fn test_structured_object_type() {
+ snowflake_and_generic().verified_stmt(
+ "SELECT payload::OBJECT(address OBJECT(city VARCHAR NOT NULL), zip
NUMBER) FROM t",
+ );
+ snowflake()
+ .verified_stmt("SELECT payload::OBJECT(tags ARRAY, labels MAP(VARCHAR,
VARCHAR)) FROM t");
+ let select = snowflake().verified_only_select(
+ "SELECT payload::OBJECT(items ARRAY(NUMBER NOT NULL), meta
MAP(VARCHAR, OBJECT(k NUMBER) NOT NULL)) FROM t",
+ );
+ let Expr::Cast { data_type, .. } =
expr_from_projection(only(&select.projection)) else {
+ unreachable!();
+ };
+ let DataType::Object(fields) = data_type else {
+ unreachable!();
+ };
+ assert!(matches!(
+ &fields[0].data_type,
+ DataType::Array(ArrayElemTypeDef::ParenthesisNotNull(_))
+ ));
+ let DataType::Map(
+ _,
+ value,
+ MapBracketKind::SnowflakeParentheses {
+ value_not_null: true,
+ },
+ ) = &fields[1].data_type
+ else {
+ unreachable!();
+ };
+ assert!(matches!(**value, DataType::Object(_)));
+
+ snowflake().one_statement_parses_to(
+ "SELECT payload::ARRAY(NUMBER) FROM t",
+ "SELECT payload::Array(NUMBER) FROM t",
+ );
+ snowflake().verified_stmt("SELECT payload::MAP(VARCHAR, OBJECT(k NUMBER))
FROM t");
+ snowflake().verified_stmt("SELECT payload::ARRAY(NUMBER NOT NULL) FROM t");
+ snowflake().verified_stmt("SELECT payload::MAP(VARCHAR, NUMBER NOT NULL)
FROM t");
+ snowflake_and_generic().verified_stmt("CREATE TABLE t (o OBJECT())");
+
+ let select = snowflake().verified_only_select(
+ "SELECT CAST(payload AS OBJECT(city VARCHAR, zip NUMBER NOT NULL))
FROM t",
+ );
+ let Expr::Cast { data_type, .. } =
expr_from_projection(only(&select.projection)) else {
+ unreachable!();
+ };
+ let DataType::Object(fields) = data_type else {
+ unreachable!();
+ };
+ assert_eq!(fields.len(), 2);
+ assert_eq!(fields[0].name, Ident::new("city"));
+ assert!(fields[0].options.is_empty());
+ assert_eq!(fields[1].name, Ident::new("zip"));
+ assert_eq!(fields[1].options.len(), 1);
+ assert_eq!(fields[1].options[0].option, ColumnOption::NotNull);
+
+ for sql in [
+ "CREATE TABLE t (o OBJECT(VARCHAR))",
+ "CREATE TABLE t (o OBJECT('json'))",
+ "CREATE TABLE t (o OBJECT('city' VARCHAR))",
+ "CREATE TABLE t (o OBJECT(city VARCHAR NULL))",
+ "CREATE TABLE t (o OBJECT(city VARCHAR)",
+ ] {
+ assert!(snowflake().parse_sql_statements(sql).is_err(), "{sql}");
+ }
Review Comment:
You should keep only the unterminated case. The other four parse as
`DataType::Custom` once the Snowflake gate is gone, which is what `main`
returns for them today.
```suggestion
assert!(snowflake()
.parse_sql_statements("CREATE TABLE t (o OBJECT(city VARCHAR)")
.is_err());
```
##########
src/parser/mod.rs:
##########
@@ -3801,15 +3801,19 @@ impl<'a> Parser<'a> {
/// ```
///
/// [map]: https://clickhouse.com/docs/en/sql-reference/data-types/map
- fn parse_click_house_map_def(&mut self) -> Result<(DataType, DataType),
ParserError> {
+ fn parse_parenthesized_map_type_def(
+ &mut self,
+ ) -> Result<(DataType, DataType, bool), ParserError> {
self.expect_keyword_is(Keyword::MAP)?;
self.expect_token(&Token::LParen)?;
let key_data_type = self.parse_data_type()?;
self.expect_token(&Token::Comma)?;
let value_data_type = self.parse_data_type()?;
+ let value_not_null = dialect_of!(self is SnowflakeDialect)
+ && self.parse_keywords(&[Keyword::NOT, Keyword::NULL]);
Review Comment:
You should gate the map value `NOT NULL` on a dialect method, mirroring
`supports_array_element_not_null()` from #2488.
```suggestion
let value_not_null = self.dialect.supports_map_value_not_null()
&& self.parse_keywords(&[Keyword::NOT, Keyword::NULL]);
```
--
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]
