Enable non-uniform field type for structs created in DataFusion #8463

dlovell · 2023-12-07T20:55:14Z

Which issue does this PR close?

Rationale for this change

Datafusion can read in data with / have structs with fields of different types, but currently can't create a struct column with fields of different types: struct creation casts of all the struct fields to the same type.

What changes are included in this PR?

This PR defines TypeSignature::VariadicLimited and assigns it as the return type of BuiltinScalarFunction::Struct. TypeSignature::VariadicLimited is like a TypeSignature::VariadicAny with a limited definition of Any and is distinct from TypeSignature::Variadic in that it does not cast any fields.

Are these changes tested?

There is a test for a successful and an unsuccessful invocation of get_valid_types for TypeSignature::VariadicLimited

Are there any user-facing changes?

The type of the generated struct column will be different: users who are expecting a cast will be surprised.

waynexia

The implementation looks good to me.

BTW, I think we also need a slt test for struct() function in the original issue

waynexia · 2023-12-08T04:37:52Z

datafusion/expr/src/signature.rs

    /// One or more arguments with arbitrary types
    VariadicAny,
+    /// arbitrary number of arguments out of a possibly different but limited set of valid types
+    VariadicLimited(Vec<DataType>),


Now we have Variadic, VariadicLimited and VariadicEqual, VariadicAny. Their behaviors are similar, and I'm wondering how we can better reflect their difference in naming.

The tail Limited also applies to Variadic. So Variadic is in actual "variadic" + "equal" + "limited", and the new one is "variadic" + "any" + "limited". How about changing Variadic to VariadicLimitedEqual and naming the new one VariadicLimitedAny? But they are a bit verbose, is there any other options?

I had similar thoughts about the ambiguity of the names and went with VariadicLimited because I didn't want to change too much.

I am pro-verbosity: tab-completion makes verbosity less painful but there is no similar thing for ambiguity.

dlovell · 2023-12-08T10:33:05Z

BTW, I think we also need a slt test for struct() function in the original issue

I wasn't sure where such a test would go or how it would look. Can you point me to an example?

dlovell · 2023-12-08T10:39:35Z

To resolve these conflicts should I upmerge main? (I started from branch-33 because I was making the changes so I could use them with datafusion-python 33.0.0.)

Conflicting files
datafusion-cli/Cargo.lock
datafusion/expr/src/built_in_function.rs

dlovell · 2023-12-08T10:45:28Z

I believe this will also close the underlying issue reported in #7012

alamb

Thank you for your contribution @dlovell . Very much appreciated.

I think any change in DataFusion needs to have some end to end tests in sqllogictests -- see https://github.com/apache/arrow-datafusion/tree/main/datafusion/sqllogictest -- perhaps you can add in the test from #8118

In this case I was thinking about it, and I don't see any reason we need to restrict the types of arguments to the struct function at all. Arrow imposes no restrictions of the types of fields in a StructArray so I don't see why DataFusion is imposing any restrictions either

When I took out all the checks for argument type like this:

diff --git a/datafusion/expr/src/built_in_function.rs b/datafusion/expr/src/built_in_function.rs
index d48e9e7a6..1a9822d76 100644
--- a/datafusion/expr/src/built_in_function.rs
+++ b/datafusion/expr/src/built_in_function.rs
@@ -959,8 +959,7 @@ impl BuiltinScalarFunction {
                 ],
                 self.volatility(),
             ),
-            BuiltinScalarFunction::Struct => Signature::variadic(
-                struct_expressions::SUPPORTED_STRUCT_TYPES.to_vec(),
+            BuiltinScalarFunction::Struct => Signature::variadic_any(
                 self.volatility(),
             ),
             BuiltinScalarFunction::Concat
diff --git a/datafusion/physical-expr/src/struct_expressions.rs b/datafusion/physical-expr/src/struct_expressions.rs
index 0eed1d16f..1d30702bb 100644
--- a/datafusion/physical-expr/src/struct_expressions.rs
+++ b/datafusion/physical-expr/src/struct_expressions.rs
@@ -34,31 +34,14 @@ fn array_struct(args: &[ArrayRef]) -> Result<ArrayRef> {
         .enumerate()
         .map(|(i, arg)| {
             let field_name = format!("c{i}");
-            match arg.data_type() {
-                DataType::Utf8
-                | DataType::LargeUtf8
-                | DataType::Boolean
-                | DataType::Float32
-                | DataType::Float64
-                | DataType::Int8
-                | DataType::Int16
-                | DataType::Int32
-                | DataType::Int64
-                | DataType::UInt8
-                | DataType::UInt16
-                | DataType::UInt32
-                | DataType::UInt64 => Ok((
+            Ok((
                     Arc::new(Field::new(
                         field_name.as_str(),
                         arg.data_type().clone(),
                         true,
                     )),
                     arg.clone(),
-                )),
-                data_type => {
-                    not_impl_err!("Struct is not implemented for type '{data_type:?}'.")
-                }
-            }
+                ))
         })
         .collect::<Result<Vec<_>>>()?;

The test case from @yukkit seems to create the field types unchanged (as verified using arrow_typeof):

DataFusion CLI v33.0.0
❯ CREATE TABLE values(
    c0 INT,
    c1 String,
    c2 String
) AS VALUES
  (1, 'a', 'a'),
  (2, 'b', 'b'),
  (3, 'c', 'c');
0 rows in set. Query took 0.015 seconds.

❯ select arrow_typeof(struct(c0, c1, c2)) from VALUES;
+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| arrow_typeof(struct(values.c0,values.c1,values.c2))                                                                                                                                                                                                                                                                                  |
+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Struct([Field { name: "c0", data_type: Int32, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }, Field { name: "c1", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }, Field { name: "c2", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }]) |
| Struct([Field { name: "c0", data_type: Int32, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }, Field { name: "c1", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }, Field { name: "c2", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }]) |
| Struct([Field { name: "c0", data_type: Int32, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }, Field { name: "c1", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }, Field { name: "c2", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }]) |
+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
3 rows in set. Query took 0.005 seconds.

Maybe @Ted-Jiang who seems to have added struct in #2389 has some contet about why the types were restricted

alamb · 2023-12-08T17:16:41Z

dev/changelog/33.0.0.md

  KIND, either express or implied.  See the License for the
  specific language governing permissions and limitations
  under the License.
 -->


I don't understand why this changelog file was modified in this PR. Perhaps it was a mistake 🤔

It seems to be some part of a change added in #8144

I was working from branch-33 so I could build datafusion-python 33.0.0 against the changes.

Should I rebase onto main?

I was working from branch-33 so I could build datafusion-python 33.0.0 against the changes.

Should I rebase onto main?

Yes please (when we are ready to merge this)

alamb · 2023-12-08T17:27:08Z

datafusion/expr/src/built_in_function.rs

                Signature::one_of(vec![VariadicAny, Any(0)], self.volatility())
            }
-            BuiltinScalarFunction::Struct => Signature::variadic(
+            BuiltinScalarFunction::Struct => Signature::variadic_limited(


What do you think about using Signature::variadic_any? That I think would pass along the argument types directly (not try to coerce them)

I think that's the simplest solution. If I make that change, I presume I should remove TypeSignature::VariadicLimited?

dlovell · 2023-12-08T19:19:05Z

I think any change in DataFusion needs to have some end to end tests in sqllogictests -- see https://github.com/apache/arrow-datafusion/tree/main/datafusion/sqllogictest -- perhaps you can add in the test from #8118

Ah, I wasn't aware of sqllogictest, looking into it.

dlovell · 2023-12-08T20:44:54Z

@alamb Looking into it, its not clear to me how @yukkit 's test is different from struct scalar function with columns #1, which already exists but also passes.

dlovell · 2023-12-08T21:50:45Z

@alamb I think I get it: I should test the explain output (which does change with the changes to struct), not the value that is written to the slt file by --complete (which does not change with the changes to struct)

alamb

Thank you @dlovell -- this looks great!

alamb · 2023-12-09T11:43:19Z

datafusion/expr/src/built_in_function.rs

                Signature::one_of(vec![VariadicAny, Any(0)], self.volatility())
            }
-            BuiltinScalarFunction::Struct => Signature::variadic(
-                struct_expressions::SUPPORTED_STRUCT_TYPES.to_vec(),


I think we can also remove SUPPORTED_STRUCT_TYPES entirely (I don't think it is used anywhere else)

We can do so as a follow on PR too.

alamb · 2023-12-09T11:45:12Z

datafusion/sqllogictest/test_files/struct.slt

 {c0: 2, c1: 2.2, c2: b}
 {c0: 3, c1: 3.3, c2: c}

+# explain struct scalar function with columns #1


👍
I double checked and before this PR this query results in casts

❯ explain select struct(a, b, c) from values; +---------------+----------------------------------------------------------------------------------------------------------------+ | plan_type | plan | +---------------+----------------------------------------------------------------------------------------------------------------+ | logical_plan | Projection: struct(CAST(values.a AS Utf8), CAST(values.b AS Utf8), values.c) | | | TableScan: values projection=[a, b, c] | | physical_plan | ProjectionExec: expr=[struct(CAST(a@0 AS Utf8), CAST(b@1 AS Utf8), c@2) as struct(values.a,values.b,values.c)] | | | MemoryExec: partitions=1, partition_sizes=[1] | | | | +---------------+----------------------------------------------------------------------------------------------------------------+ 2 rows in set. Query took 0.009 seconds.

alamb · 2023-12-09T11:48:51Z

@dlovell -- this PR has conflicts that need to be resolved before I can merge into main (basically we need to rebase this PR against main)

Is that something you would like to do?

…d types

dlovell · 2023-12-09T12:21:58Z

@alamb I did the rebase, let me know if there's anything that needs to be changed.

alamb · 2023-12-09T12:25:16Z

Thanks again @dlovell -- BTW I love bug fixes that involve deleting code ❤️

alamb · 2023-12-09T12:32:03Z

🤔 looks like we need to run cargo fmt and cargo clippy to get a clean CI run

yukkit · 2023-12-09T14:48:31Z

This looks great!

dlovell · 2023-12-09T15:44:02Z

@alamb I ran cargo fmt and there was a change, but no change for cargo clippy

dlovell · 2023-12-09T15:44:17Z

@alamb I ran cargo fmt and there was a change, but no change for cargo clippy

dlovell · 2023-12-10T10:42:44Z

@alamb apologies, just found out about dev/rust_lint.sh

alamb · 2023-12-10T14:21:43Z

No worries -- thanks for sticking with it @dlovell

…he#8463) * feat: struct: implement variadic_any solution, enable all struct field types * fix: run cargo-fmt * cln: remove unused imports

github-actions bot added the logical-expr Logical plan and expressions label Dec 7, 2023

alamb mentioned this pull request Dec 7, 2023

DataFusion weekly project plan (Andrew Lamb) - Dec 4, 2023 #8420

Closed

7 tasks

waynexia approved these changes Dec 8, 2023

View reviewed changes

alamb reviewed Dec 8, 2023

View reviewed changes

github-actions bot added physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Dec 8, 2023

alamb approved these changes Dec 9, 2023

View reviewed changes

dlovell force-pushed the variadic-limited-33 branch from 2536cda to 4474b07 Compare December 9, 2023 12:05

feat: struct: implement variadic_any solution, enable all struct fiel…

323ceb0

…d types

dlovell force-pushed the variadic-limited-33 branch from 4474b07 to 323ceb0 Compare December 9, 2023 12:07

alamb mentioned this pull request Dec 9, 2023

Minor: Remove now dead SUPPORTED_STRUCT_TYPES #8480

Merged

fix: run cargo-fmt

e1dc180

cln: remove unused imports

97b99db

alamb merged commit 93b21bd into apache:main Dec 11, 2023

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

Enable non-uniform field type for structs created in DataFusion #8463

Enable non-uniform field type for structs created in DataFusion #8463

Uh oh!

Conversation

dlovell commented Dec 7, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

waynexia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlovell commented Dec 8, 2023

Uh oh!

dlovell commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlovell commented Dec 8, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlovell commented Dec 8, 2023

Uh oh!

dlovell commented Dec 8, 2023

Uh oh!

dlovell commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Dec 9, 2023

Uh oh!

dlovell commented Dec 9, 2023

Uh oh!

alamb commented Dec 9, 2023

Uh oh!

alamb commented Dec 9, 2023

Uh oh!

yukkit commented Dec 9, 2023

Uh oh!

dlovell commented Dec 9, 2023

Uh oh!

dlovell commented Dec 9, 2023

Uh oh!

dlovell commented Dec 10, 2023

Uh oh!

alamb commented Dec 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

dlovell commented Dec 8, 2023 •

edited

Loading

alamb Dec 8, 2023 •

edited

Loading

dlovell commented Dec 8, 2023 •

edited

Loading

alamb commented Dec 10, 2023 •

edited

Loading