feat: Enforce the uniqueness of map key name for the map/make_map function

[Weijun-H]

Which issue does this PR close?

Closes #11437

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

[goldmedal]

Thanks @Weijun-H overall LGTM. I also tested some cases for nested type as the key. They work well. 👍

# Test duplicate keys
query error DataFusion error: Execution error: map key must be unique, duplicate key found: \[1, 2\]
SELECT MAP([[1,2], [1,2], []], [41, 33, null]);

# Test duplicate keys
query error DataFusion error: Execution error: map key must be unique, duplicate key found: \[\{1:1\}\]
SELECT MAP([Map {1:'1'}, Map {1:'1'}, Map {2:'2'}], [41, 33, null]);

[Weijun-H]

Thanks @Weijun-H overall LGTM. I also tested some cases for nested type as the key. They work well. 👍

# Test duplicate keys
query error DataFusion error: Execution error: map key must be unique, duplicate key found: \[1, 2\]
SELECT MAP([[1,2], [1,2], []], [41, 33, null]);

# Test duplicate keys
query error DataFusion error: Execution error: map key must be unique, duplicate key found: \[\{1:1\}\]
SELECT MAP([Map {1:'1'}, Map {1:'1'}, Map {2:'2'}], [41, 33, null]);

Nice catch! I added them to the tests

[dharanad]

I plan to review this today

[dharanad]

Can we use datafusion_common::utils::list_to_arrays::<O>(keys) here ?
Also any specific reason for making this a inner fn ? why not move it out ? For better readability

[dharanad]

Look good. But suggested some minor change for better code readability

[Weijun-H]

@jayzhan211 do you have some time to review this pr?

[jayzhan211]

Will it be faster if we check early?

    fn plan_make_map(&self, args: Vec<Expr>) -> Result<PlannerResult<Vec<Expr>>> {
        if args.len() % 2 != 0 {
            return plan_err!("make_map requires an even number of arguments");
        }

        let (keys, values): (Vec<_>, Vec<_>) =
            args.into_iter().enumerate().partition(|(i, _)| i % 2 == 0);
       // check here?
        let keys = make_array(keys.into_iter().map(|(_, e)| e).collect());
        let values = make_array(values.into_iter().map(|(_, e)| e).collect());

        Ok(PlannerResult::Planned(Expr::ScalarFunction(
            ScalarFunction::new_udf(map_udf(), vec![keys, values]),
        )))
    }

pub fn map(keys: Vec<Expr>, values: Vec<Expr>) -> Expr {
    // check here
    let keys = make_array(keys);
    let values = make_array(values);
    Expr::ScalarFunction(ScalarFunction::new_udf(map_udf(), vec![keys, values]))
}

[jayzhan211]

We could have a function for these two

        fn create_map_from_keys_and_values(keys: Vec<Expr>, values: Vec<Expr>) -> Expr {
            // check keys
            let keys = make_array(keys);
            let values = make_array(values);
            map_udf().call(vec![keys, values])
        }

[jayzhan211]

Another benefit is that we can have map_with_checks version that avoid unique key check

[Weijun-H]

We could check for unique keys earlier in the make_map function, but since the MAP function is a UDF, the unique key validation needs to be handled in the make_map_batch function. This approach, however, will result in the make_map function performing the check twice.

[jayzhan211]

The uniqueness is guaranteed in semantic layer, not in the map internal function. We don't need to check again in the internal function. Consider that if someone want map function that allows duplicate keys, the check you did blocks their requirement.

[Weijun-H]

Yes, however, it cannot guarantee this query.

External error: The query was expected to fail but actually succeeded:
[SQL] SELECT MAP(['POST', 'HEAD', 'POST'], [41, 33, NULL]);

According to DuckDB behavior, this should have failed because the keys in the map are not unique.

[jayzhan211]

I found my approach doesn't work for column, we could only check them in invoke

[jayzhan211]

👍

[Weijun-H]

Thank you @dharanad @goldmedal @jayzhan211