Add support for DuckDB struct literal syntax

[gstvg]

Fixes#1129

Existing Struct expression supports the BigQuery semantics, which allow for optionally typed fields, that if typed, also have an optional name.
DuckDB has incompatible semantics: only named fields without any type specification.

Should I create a separate expression, perharps NamedFieldsStruct, or change the Struct to have an inner enum Struct(enum { MaybeTyped(...), Named(...) }) ?

Thanks!

[jayzhan211]

You can ask the question in issue. The PR draft doesn't send the notification.

Let me reference the PR apache/datafusion#9743

[coveralls]

Pull Request Test Coverage Report for Build 8480512054

Details

• 74 of 74(100.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.04%) to 87.92%

---

Totals
Change from base Build 8411261926:	0.04%
Covered Lines:	20560
Relevant Lines:	23385

---

💛 - Coveralls

[alamb]

Should I create a separate expression, perharps NamedFieldsStruct, or change the Struct to have an inner enum Struct(enum { MaybeTyped(...), Named(...) }) ?

I think given the difference in syntax, having a new struct like NamedFieldsStruct would be better / less confusing / easier to handle

Thank you again for working on this @gstvg and @jayzhan211

[gstvg]

thanks @jayzhan211 and @alamb
I created a new Expr named Dictionary, which currently only supports struct literals.

I tried adding supporting for DuckDB MAP too, but since map keys can be any expression, it conflicted with Snowflake-only JsonAccess with colon json_field:key, which is currently parsed for all dialects. Restricting it to Snowflake solves the problem. Should i push it here PR too?

[jayzhan211]

thanks @jayzhan211 and @alamb I created a new Expr named Dictionary, which currently only supports struct literals.

I tried adding supporting for DuckDB MAP too, but since map keys can be any expression, it conflicted with Snowflake-only JsonAccess with colon json_field:key, which is currently parsed for all dialects. Restricting it to Snowflake solves the problem. Should i push it here PR too?

I don't understand what you mean by Snowflake-only JsonAccess with colon json_field:key, can you elaborate it more.
It seems the difference between struct and map in Duckdb is that the key_type can be different for map in each row. They are both json-like objects. Snowflake map's key type should be string or number, nested types for key is not allowed

which is currently parsed for all dialects

I think we have not yet supported any.

[gstvg]

Sure. I would like to solve all this at once, to be honest.

Snowflake JSON access uses : to query data

https://docs.snowflake.com/en/user-guide/json-basics-tutorial-query

DuckDB maps key can be any type, not only strings, but may have different set of keys for each row

https://duckdb.org/docs/sql/data_types/map.html

To support any type of key, I used Parser::parse_expr instead of parse_identifier

Currently, because of the following arms in parse_infix and get_next_precedence, when parse_expr is called to get the key expr, it parses the whole key: value as a JsonAccess

If those two arms are moved to the Snowflake dialect, or conditioned to a dialect_of!(Snowflake), parse_expr will return the key only, not the whole key value, but the json_column:json_key would be restricted to the Snowflake dialect

I didn't find any other way to do that

I've pushed it to another branch if you have interest

which is currently parsed for all dialects

I think we have not yet supported any.

Sorry, I mean the json_column:json_key is parsed for all dialects, not restricted to the Generic and/or Snowflake

[gstvg]

Others dialects that uses this syntax:

Snowflake

Syntax limited to Object constants, so parse_value or parse_identifier can be called instead of parse_expr

ClickHouse

Has the same conflicts as DuckDB map, which also means it has the same solution 😀

[jayzhan211]

How about considering Expr::JsonAcess as your Expr::DictionaryField, it seems to me they are equivalent but the json operator is colon. Similar to parsing [e1, e2...] but {e1, e2...} instead.

Expr::Dictionary(Vec<Expr>), where we can get Expr::JsonAccess (left colon right)

[jayzhan211]

How about considering Expr::JsonAcess as your Expr::DictionaryField, it seems to me they are equivalent but the json operator is colon. Similar to parsing [e1, e2...] but {e1, e2...} instead.

Expr::Dictionary(Vec<Expr>), where we can get Expr::JsonAccess (left colon right)

I met the issue while trying out my idea #1197

[alamb]

I think this looks good to me: the use of parse_expr and restricting it to DuckDB makes sense to me

If/When people need support for this type in Snowflake and the other dialects, I think we can handle them separately

[alamb]

I also pushed d9d16ce to fix the doc test failues

[alamb]

@jayzhan211 does this change look good for you?

[jayzhan211]

I think either struct and map and other dictionaries like syntax {"key": "value"} is possible to share the same parsing rule.
It is fine to do it specifically for now, but I think we can share the similarity for other dialects or syntax later on.

[alamb]

Thanks @jayzhan211 and @gstvg !

[gstvg]

How about considering Expr::JsonAcess as your Expr::DictionaryField, it seems to me they are equivalent but the json operator is colon. Similar to parsing [e1, e2...] but {e1, e2...} instead.

Expr::Dictionary(Vec<Expr>), where we can get Expr::JsonAccess (left colon right)

Good idea. But making Dictionary a Vec<Expr> wouldn't be unergonomic for users? Extending it, what about something like this:

 fn parse_duckdb_dictionary_field(&mut self) -> Result<DictionaryField, ParserError> {
if let Expr::JsonAccess { left, operator: JsonOperator::Colon, right } = self.parse_expr()? {
Ok(DictionaryField {
key: left,
value: right,
})
} else {
self.expected(":", self.peek_token())
}
}

[jayzhan211]

How about considering Expr::JsonAcess as your Expr::DictionaryField, it seems to me they are equivalent but the json operator is colon. Similar to parsing [e1, e2...] but {e1, e2...} instead.
Expr::Dictionary(Vec<Expr>), where we can get Expr::JsonAccess (left colon right)

Good idea. But making Dictionary a Vec<Expr> wouldn't be unergonomic for users? Extending it, what about something like this:

 fn parse_duckdb_dictionary_field(&mut self) -> Result<DictionaryField, ParserError> {
if let Expr::JsonAccess { left, operator: JsonOperator::Colon, right } = self.parse_expr()? {
Ok(DictionaryField {
key: left,
value: right,
})
} else {
self.expected(":", self.peek_token())
}
}

It looks good to me.