Support planning Map literal
#11780
Support planning Map literal
#11780
Conversation
| # TODO: support parsing an empty map | ||
| # query ? | ||
| # SELECT MAP {}; | ||
| # ---- | ||
| # {} |
There was a problem hiding this comment.
I missed supporting this in sqlparser-rs. I'll have PR for it.
There was a problem hiding this comment.
I have created a PR for it sqlparser-rs/sqlparser-rs#1361
datafusion/sql/src/expr/mod.rs
Outdated
| PlannerResult::Original(expr) => exprs = expr, | ||
| } | ||
| } | ||
| not_impl_err!("Unsupported map literal: {exprs:?}") |
There was a problem hiding this comment.
| not_impl_err!("Unsupported map literal: {exprs:?}") | |
| not_impl_err!("Unsupported MAP literal: {exprs:?}") |
| # {} | ||
|
|
||
| query error DataFusion error: Execution error: map key cannot be null | ||
| SELECT MAP {'a': 1, null: 2} |
There was a problem hiding this comment.
can we add some more negative and nested cases?
Map of map, map with values null, map with different types?
There was a problem hiding this comment.
Imho it is great improvement thanks @goldmedal . Lets add some more tests on literal just to make sure the literal is ready to go.
| # TODO: fix accessing map with non-string key | ||
| # query ? | ||
| # SELECT MAP { 1: 'a', 2: 'b', 3: 'c' }[1]; | ||
| # ---- | ||
| # a | ||
|
|
||
| # TODO: fix accessing map with non-string key | ||
| # query ? | ||
| # SELECT MAP { MAP {1:'a', 2:'b'}:1, MAP {1:'c', 2:'d'}:2 }[MAP {1:'a', 2:'b'}]; | ||
| # ---- | ||
| # 1 | ||
|
|
||
| # TODO: fix accessing map with non-string key | ||
| # query ? | ||
| # SELECT MAKE_MAP(1, null, 2, 33, 3, null)[2]; | ||
| # ---- | ||
| # 33 |
There was a problem hiding this comment.
I found some issues about accessing a map with a non-string key but I think we can fix it in another PR.
There was a problem hiding this comment.
Filed #11785 for them.
There was a problem hiding this comment.
Yeah, thats the entire idea of adding tests to find potential issues.
please add the issue tracker link to the commented out tests
datafusion/sql/src/expr/mod.rs
Outdated
| let mut exprs = vec![]; | ||
| entries.into_iter().try_for_each(|entry| { | ||
| exprs.push(self.sql_expr_to_logical_expr( | ||
| *entry.key, | ||
| schema, | ||
| planner_context, | ||
| )?); | ||
| exprs.push(self.sql_expr_to_logical_expr( | ||
| *entry.value, | ||
| schema, | ||
| planner_context, | ||
| )?); | ||
| Ok::<_, DataFusionError>(()) | ||
| })?; |
There was a problem hiding this comment.
Can we do something like this ?
let mut exprs = entries.into_iter().flat_map(|entry| {
let k = self.sql_expr_to_logical_expr(
*entry.key,
schema,
planner_context,
)?;
let v = self.sql_expr_to_logical_expr(
*entry.value,
schema,
planner_context,
)?;
vec![k,v]
}).collect<>();;Haven't tried it out just a pseudo code
There was a problem hiding this comment.
I think it's wrong. What plan_make_map accepts is [k1, v1, k2, v2, ... ]. The parameters will be arranged to 2 vectors for map_udf.
If we change it to [[k1, k2, ...], [v1, v2, ...], it will become an array-to-array map, MAP {[k1, k2, ...]: [v1, v2, ...]}.
There was a problem hiding this comment.
Like i mentioned that was just pseudo code (i should have called it a rough idea). I was try to suggest something like this
let arr = (1..10).step_by(2).into_iter().flat_map(|e| vec![e,e+1]).collect::<Vec<_>>();
[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]If we can do something like this the expressions iter can be rearrage to [k1, v1, k2, v2, ... ]
There was a problem hiding this comment.
Got it. I tried to simplify them in e8a3477. It looks better. Thanks.
datafusion/sql/src/expr/mod.rs
Outdated
| PlannerResult::Original(expr) => exprs = expr, | ||
| } | ||
| } | ||
| not_impl_err!("Unsupported MAP literal: {exprs:?}") |
There was a problem hiding this comment.
Can we change it to MAP not supported by ExprPlanner: {exprs:?} to maintains consistency with other errors in the file.
There was a problem hiding this comment.
I also align the error message for Dictionary in ea38d6b.
There was a problem hiding this comment.
This is a beautiful PR @goldmedal
Thank you @comphead and @dharanad for the review (the PR looks great now)
Which issue does this PR close?
Closes #11434
Rationale for this change
What changes are included in this PR?
Are these changes tested?
yes
Are there any user-facing changes?
Provide
mapliteral syntax.