Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support planning Map literal #11780

Merged
merged 6 commits into from
Aug 3, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 38 additions & 4 deletions datafusion/sql/src/expr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ use datafusion_expr::planner::PlannerResult;
use datafusion_expr::planner::RawDictionaryExpr;
use datafusion_expr::planner::RawFieldAccessExpr;
use sqlparser::ast::{
CastKind, DictionaryField, Expr as SQLExpr, StructField, Subscript, TrimWhereField,
Value,
CastKind, DictionaryField, Expr as SQLExpr, MapEntry, StructField, Subscript,
TrimWhereField, Value,
};

use datafusion_common::{
internal_datafusion_err, internal_err, not_impl_err, plan_err, DFSchema, Result,
ScalarValue,
internal_datafusion_err, internal_err, not_impl_err, plan_err, DFSchema,
DataFusionError, Result, ScalarValue,
};
use datafusion_expr::expr::InList;
use datafusion_expr::expr::ScalarFunction;
Expand Down Expand Up @@ -628,6 +628,9 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
SQLExpr::Dictionary(fields) => {
self.try_plan_dictionary_literal(fields, schema, planner_context)
}
SQLExpr::Map(map) => {
self.try_plan_map_literal(map.entries, schema, planner_context)
}
_ => not_impl_err!("Unsupported ast node in sqltorel: {sql:?}"),
}
}
Expand Down Expand Up @@ -714,6 +717,37 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
not_impl_err!("Unsupported dictionary literal: {raw_expr:?}")
}

fn try_plan_map_literal(
&self,
entries: Vec<MapEntry>,
schema: &DFSchema,
planner_context: &mut PlannerContext,
) -> Result<Expr> {
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>(())
})?;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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, ...]}.

Copy link
Contributor

Choose a reason for hiding this comment

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

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, ... ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I tried to simplify them in e8a3477. It looks better. Thanks.

for planner in self.context_provider.get_expr_planners() {
match planner.plan_make_map(exprs)? {
PlannerResult::Planned(expr) => {
return Ok(expr);
}
PlannerResult::Original(expr) => exprs = expr,
}
}
not_impl_err!("Unsupported MAP literal: {exprs:?}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change it to MAP not supported by ExprPlanner: {exprs:?} to maintains consistency with other errors in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also align the error message for Dictionary in ea38d6b.

}

// Handles a call to struct(...) where the arguments are named. For example
// `struct (v as foo, v2 as bar)` by creating a call to the `named_struct` function
fn create_named_struct_expr(
Expand Down
149 changes: 149 additions & 0 deletions datafusion/sqllogictest/test_files/map.slt
Original file line number Diff line number Diff line change
Expand Up @@ -310,3 +310,152 @@ VALUES (MAP(['a'], [1])), (MAP(['b'], [2])), (MAP(['c', 'a'], [3, 1]))
{a: 1}
{b: 2}
{c: 3, a: 1}

query ?
SELECT MAP {'a':1, 'b':2, 'c':3};
----
{a: 1, b: 2, c: 3}

query ?
SELECT MAP {'a':1, 'b':2, 'c':3 } FROM t;
----
{a: 1, b: 2, c: 3}
{a: 1, b: 2, c: 3}
{a: 1, b: 2, c: 3}

query I
SELECT MAP {'a':1, 'b':2, 'c':3}['a'];
----
1

query I
SELECT MAP {'a':1, 'b':2, 'c':3 }['a'] FROM t;
----
1
1
1

# TODO: support parsing an empty map
# query ?
# SELECT MAP {};
# ----
# {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed supporting this in sqlparser-rs. I'll have PR for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a PR for it sqlparser-rs/sqlparser-rs#1361


# values contain null
query ?
SELECT MAP {'a': 1, 'b': null};
----
{a: 1, b: }

# keys contain null
query error DataFusion error: Execution error: map key cannot be null
SELECT MAP {'a': 1, null: 2}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add some more negative and nested cases?

Map of map, map with values null, map with different types?


# array as key
query ?
SELECT MAP {[1,2,3]:1, [2,4]:2};
----
{[1, 2, 3]: 1, [2, 4]: 2}

# array with different type as key
# expect to fail due to type coercion error
query error
SELECT MAP {[1,2,3]:1, ['a', 'b']:2};

# array as value
query ?
SELECT MAP {'a':[1,2,3], 'b':[2,4]};
----
{a: [1, 2, 3], b: [2, 4]}

# array with different type as value
# expect to fail due to type coercion error
query error
SELECT MAP {'a':[1,2,3], 'b':['a', 'b']};

# struct as key
query ?
SELECT MAP {{'a':1, 'b':2}:1, {'a':3, 'b':4}:2};
----
{{a: 1, b: 2}: 1, {a: 3, b: 4}: 2}

# struct with different fields as key
# expect to fail due to type coercion error
query error
SELECT MAP {{'a':1, 'b':2}:1, {'c':3, 'd':4}:2};

# struct as value
query ?
SELECT MAP {'a':{'b':1, 'c':2}, 'b':{'b':3, 'c':4}};
----
{a: {b: 1, c: 2}, b: {b: 3, c: 4}}

# struct with different fields as value
# expect to fail due to type coercion error
query error
SELECT MAP {'a':{'b':1, 'c':2}, 'b':{'c':3, 'd':4}};

# map as key
query ?
SELECT MAP { MAP {1:'a', 2:'b'}:1, MAP {1:'c', 2:'d'}:2 };
----
{{1: a, 2: b}: 1, {1: c, 2: d}: 2}

# map with different keys as key
query ?
SELECT MAP { MAP {1:'a', 2:'b', 3:'c'}:1, MAP {2:'c', 4:'d'}:2 };
----
{{1: a, 2: b, 3: c}: 1, {2: c, 4: d}: 2}

# map as value
query ?
SELECT MAP {1: MAP {1:'a', 2:'b'}, 2: MAP {1:'c', 2:'d'} };
----
{1: {1: a, 2: b}, 2: {1: c, 2: d}}

# map with different keys as value
query ?
SELECT MAP {'a': MAP {1:'a', 2:'b', 3:'c'}, 'b': MAP {2:'c', 4:'d'} };
----
{a: {1: a, 2: b, 3: c}, b: {2: c, 4: d}}

# complex map for each row
query ?
SELECT MAP {'a': MAP {1:'a', 2:'b', 3:'c'}, 'b': MAP {2:'c', 4:'d'} } from t;
----
{a: {1: a, 2: b, 3: c}, b: {2: c, 4: d}}
{a: {1: a, 2: b, 3: c}, b: {2: c, 4: d}}
{a: {1: a, 2: b, 3: c}, b: {2: c, 4: d}}

# access map with non-existent key
query ?
SELECT MAP {'a': MAP {1:'a', 2:'b', 3:'c'}, 'b': MAP {2:'c', 4:'d'} }['c'];
----
NULL

# access map with null key
query error
SELECT MAP {'a': MAP {1:'a', 2:'b', 3:'c'}, 'b': MAP {2:'c', 4:'d'} }[NULL];

query ?
SELECT MAP { 'a': 1, 2: 3 };
----
{a: 1, 2: 3}

# 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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found some issues about accessing a map with a non-string key but I think we can fix it in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #11785 for them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, thats the entire idea of adding tests to find potential issues.
please add the issue tracker link to the commented out tests

Loading