Support parsing empty map literal syntax for DuckDB and Genric #1361

goldmedal · 2024-08-02T13:32:37Z

Description

DuckDB supports creating an empty map like

D select map {};
┌────────────────────────────────────────────────┐
│ main.map(main.list_value(), main.list_value()) │
│             map(integer, integer)              │
├────────────────────────────────────────────────┤
│ {}                                             │
└────────────────────────────────────────────────┘

coveralls · 2024-08-02T13:37:05Z

Pull Request Test Coverage Report for Build 10235175469

Details

11 of 11 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.001%) to 89.235%

Totals
Change from base Build 10206043624:	0.001%
Covered Lines:	27472
Relevant Lines:	30786

💛 - Coveralls

dharanad · 2024-08-02T17:28:04Z

src/parser/mod.rs

-
-        Ok(Expr::Map(Map { entries: fields }))
+        if self.peek_token().token == Token::RBrace {
+            let _ = self.next_token(); // consume }


Nit: ambiguous comments

I followed the same style as parsing the array literal. Should I enhance them?
https://github.com/sqlparser-rs/sqlparser-rs/blob/d49acc67b13e1d68f2e6a25546161a68e165da4f/src/parser/mod.rs#L1853

My bad, i interpreted wrongly. This look good.

iffyio · 2024-08-03T04:57:13Z

src/parser/mod.rs

-        self.expect_token(&Token::RBrace)?;
-
-        Ok(Expr::Map(Map { entries: fields }))
+        if self.peek_token().token == Token::RBrace {


is it possible to switch to let fields = self.parse_comma_separated0(Self::parse_duckdb_map_field)?; instead? if so that would let us skip the if/else here

I don't think we can do it. It seems that parse_comma_separated0 is for parentheses, but MAP uses braces.
https://github.com/sqlparser-rs/sqlparser-rs/blob/d49acc67b13e1d68f2e6a25546161a68e165da4f/src/parser/mod.rs#L3487

Ah i see, I'm thinking we could still reuse the logic (thinking its currently a bit odd that parse_comma_separated0 is hardcoded to parenthesis while parse_comma_separated is token agnostic as it should be). Seems there's ony a couple of usages of parse_comma_separated0, we could change it to something like parse_comma_separated0(|| {}, Token::RParen)?

I addressed this in 0763f52 and 5d81654. I share the same logic for brace, parenthesis, and bracket. I think it looks better. Thanks.

iffyio · 2024-08-04T04:40:20Z

src/parser/mod.rs

+    pub fn parse_comma_separated0<T, F>(
+        &mut self,
+        f: F,
+        trailing_commas: bool,


hmm I'm not too sure if pulling out the trailing_comma option is worth it/desirable - the current behavior isn't specified on the dialect level, but rather on the parser itself, to be overridden by the user, and I wonder if it'll get confusing to have to figure out where each dialect supports or doesnt support trailing commas. Would it be okay to keep the behavior as it was before? (i.e. always guarding internally on self.options.trailing_comma)

hmm I'm not too sure if pulling out the trailing_comma option is worth it/desirable - the current behavior isn't specified on the dialect level, but rather on the parser itself, to be overridden by the user, and I wonder if it'll get confusing to have to figure out where each dialect supports or doesnt support trailing commas. Would it be okay to keep the behavior as it was before? (i.e. always guarding internally on self.options.trailing_comma)

I see. I tried more cases for DuckDB, I found the trailing_comma behaviors are shared.

D select * from t where a in (1,2,); ┌────────┐ │ a │ │ int32 │ ├────────┤ │ 0 rows │ └────────┘ D select {'a':1,}; ┌──────────────────────────┐ │ main.struct_pack(a := 1) │ │ struct(a integer) │ ├──────────────────────────┤ │ {'a': 1} │ └──────────────────────────┘ D select [1,2,]; ┌───────────────────────┐ │ main.list_value(1, 2) │ │ int32[] │ ├───────────────────────┤ │ [1, 2] │ └───────────────────────┘

I think we can use self.options.trailing_comma internally.

iffyio · 2024-08-04T04:36:24Z

src/parser/mod.rs

-            self.expect_token(&Token::RBracket)?;
-            Ok(Expr::Array(Array { elem: exprs, named }))
-        }
+        let exprs = self.parse_comma_separated0(Parser::parse_expr, false, Token::RBracket)?;


do we know if this change is going to be correct for all dialects? Otherwise we can probably keep the behavior to match the parser configuration

I'm not sure but all the tests are passed. The logic is the same as what I did for the map literal (actually, I followed it to implement the empty map). Should I roll back it?

iffyio · 2024-08-04T04:40:20Z

src/parser/mod.rs

+    pub fn parse_comma_separated0<T, F>(
+        &mut self,
+        f: F,
+        trailing_commas: bool,


hmm I'm not too sure if pulling out the trailing_comma option is worth it/desirable - the current behavior isn't specified on the dialect level, but rather on the parser itself, to be overridden by the user, and I wonder if it'll get confusing to have to figure out where each dialect supports or doesnt support trailing commas. Would it be okay to keep the behavior as it was before? (i.e. always guarding internally on self.options.trailing_comma)

iffyio

LGTM! cc @alamb

alamb · 2024-08-04T11:47:17Z

src/parser/mod.rs

+    pub fn parse_comma_separated0<T, F>(
+        &mut self,
+        f: F,
+        end_token: Token,


❤️ that is a nice generalization

alamb

Thanks @goldmedal and @iffyio

alamb · 2024-08-04T11:47:43Z

(and @git-hulk and @dharanad 🤯 )

…e#1361)

support parsing empty map literal syntax

21cdef3

goldmedal mentioned this pull request Aug 2, 2024

Support planning Map literal apache/datafusion#11780

Merged

git-hulk approved these changes Aug 2, 2024

View reviewed changes

dharanad reviewed Aug 2, 2024

View reviewed changes

dharanad approved these changes Aug 2, 2024

View reviewed changes

iffyio reviewed Aug 3, 2024

View reviewed changes

goldmedal added 3 commits August 3, 2024 22:17

extract parse_comma_separated0

0763f52

use parse_comma_separated0 for parsing array and map elements

5d81654

fix the doc

4479ecd

goldmedal commented Aug 3, 2024

View reviewed changes

iffyio reviewed Aug 4, 2024

View reviewed changes

roll back trailing_commas

239ef53

iffyio approved these changes Aug 4, 2024

View reviewed changes

alamb reviewed Aug 4, 2024

View reviewed changes

src/parser/mod.rs

pub fn parse_comma_separated0<T, F>(

&mut self,

f: F,

end_token: Token,

Copy link

Contributor

alamb Aug 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ that is a nice generalization

alamb approved these changes Aug 4, 2024

View reviewed changes

alamb merged commit 8f8c96f into apache:main Aug 4, 2024
10 checks passed

goldmedal deleted the support-empty-map branch August 4, 2024 14:09

goldmedal mentioned this pull request Aug 26, 2024

Enable the test for creating empty map apache/datafusion#12176

Merged

ayman-sigma pushed a commit to sigmacomputing/sqlparser-rs that referenced this pull request Nov 19, 2024

Support parsing empty map literal syntax for DuckDB and Genric (apach…

32b6609

…e#1361)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support parsing empty map literal syntax for DuckDB and Genric #1361

Support parsing empty map literal syntax for DuckDB and Genric #1361

goldmedal commented Aug 2, 2024

coveralls commented Aug 2, 2024 •

edited

Loading

dharanad Aug 2, 2024

goldmedal Aug 2, 2024

dharanad Aug 2, 2024

iffyio Aug 3, 2024

goldmedal Aug 3, 2024

iffyio Aug 3, 2024

goldmedal Aug 3, 2024

This comment was marked as outdated.

iffyio Aug 4, 2024

goldmedal Aug 4, 2024

iffyio Aug 4, 2024

goldmedal Aug 4, 2024

iffyio Aug 4, 2024

iffyio left a comment

alamb Aug 4, 2024

alamb left a comment

alamb commented Aug 4, 2024

Support parsing empty map literal syntax for DuckDB and Genric #1361

Support parsing empty map literal syntax for DuckDB and Genric #1361

Conversation

goldmedal commented Aug 2, 2024

Description

coveralls commented Aug 2, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10235175469

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb commented Aug 4, 2024

coveralls commented Aug 2, 2024 •

edited

Loading