Jefffrey commented on code in PR #22784:
URL: https://github.com/apache/datafusion/pull/22784#discussion_r3373851746
##########
datafusion/functions-nested/src/map.rs:
##########
@@ -904,4 +938,70 @@ mod tests {
assert!(map_array.is_null(1), "Second map should be NULL");
assert!(!map_array.is_null(2), "Third map should not be NULL");
}
+
+ #[test]
+ fn test_make_map_scalar_keys_array_values() {
Review Comment:
i think the SLT is sufficient, dont need to duplicate here
##########
datafusion/sqllogictest/test_files/map.slt:
##########
@@ -901,3 +901,14 @@ NULL
statement ok
drop table tt;
+
+# Regression test: map with literal keys and column-reference values
+# Previously failed with "map requires key and value lists to have the same
length"
+# because scalar keys (FixedSizeList) had length=N (number of keys) while
+# array values had length=batch_size.
Review Comment:
```suggestion
# mixed scalar/array inputs
```
better to keep it short and simple
##########
datafusion/functions-nested/src/map.rs:
##########
@@ -63,15 +63,37 @@ fn can_evaluate_to_const(args: &[ColumnarValue]) -> bool {
.all(|arg| matches!(arg, ColumnarValue::Scalar(_)))
}
-fn make_map_batch(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+fn make_map_batch(args: &[ColumnarValue], number_rows: usize) ->
Result<ColumnarValue> {
let [keys_arg, values_arg] = take_function_args("make_map", args)?;
let can_evaluate_to_const = can_evaluate_to_const(args);
- let keys = get_first_array_ref(keys_arg)?;
+ // If we have a mix of scalar and array args, expand the scalar to an array
+ // so that the rest of the logic handles uniform array inputs.
+ // This handles cases like: map(['a','b'], [col1, col2]) where keys are all
+ // literals (scalar FixedSizeList) but values contain column references.
Review Comment:
```suggestion
// if we can't evaluate to const (inputs are not both scalar) then
ensure they
// are expanded to arrays which following logic expects
```
simplifying comment a bit
##########
datafusion/functions-nested/src/map.rs:
##########
@@ -63,15 +63,37 @@ fn can_evaluate_to_const(args: &[ColumnarValue]) -> bool {
.all(|arg| matches!(arg, ColumnarValue::Scalar(_)))
}
-fn make_map_batch(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+fn make_map_batch(args: &[ColumnarValue], number_rows: usize) ->
Result<ColumnarValue> {
let [keys_arg, values_arg] = take_function_args("make_map", args)?;
let can_evaluate_to_const = can_evaluate_to_const(args);
- let keys = get_first_array_ref(keys_arg)?;
+ // If we have a mix of scalar and array args, expand the scalar to an array
+ // so that the rest of the logic handles uniform array inputs.
+ // This handles cases like: map(['a','b'], [col1, col2]) where keys are all
+ // literals (scalar FixedSizeList) but values contain column references.
+ let (keys_arg, values_arg) = if !can_evaluate_to_const {
+ let keys_arg = match keys_arg {
+ ColumnarValue::Scalar(s) => {
+ ColumnarValue::Array(s.to_array_of_size(number_rows)?)
+ }
+ other => other.clone(),
+ };
+ let values_arg = match values_arg {
+ ColumnarValue::Scalar(s) => {
+ ColumnarValue::Array(s.to_array_of_size(number_rows)?)
Review Comment:
it would be nice if we could refactor the make map logic as a whole to not
have this confusing expansion to array but still wrap in columnarvalue, but i
suppose that would require a larger refactor
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
