Support Utf8View and BinaryView in substrait serialization. #12199

wiedld · 2024-08-27T18:04:22Z

Which issue does this PR close?

Rationale for this change

We have two new view data types, Utf8View and BinaryView. Support in datafusion is part of this epic, and this specific PR is about adding support for the (de-)serialization of logical and physical plans into the substrait format.

This PR adds new substrait variations on existing type classes. For example, there is a "string" substrait class which can have different variations representing different physical types (e.g. Utf8 vs LargeUtf8 vs Utf8View). If we serialize using string variation=2 (e.g. view physical type), then the deserialization of variation=2 will give us back the Utf8View. More background is given here.

What changes are included in this PR?

feat(12118): logical plan support for Utf8View (d7be771)
feat(12118): physical plan support for Utf8View (b17ae25)
feat(12118): logical plan support for BinaryView (f38085d)
feat(12118): physical plan support for BinaryView (5c4ebec)

Are these changes tested?

Logical plan: The Utf8View and BinaryView are covered in the logical plan roundtrip serialization tests.

Physical plan: However, the physical plan roundtrip serialization tests are not yet implemented. There is an ongoing epic to finish the physical plan serialization. As such, I added code for the physical plan substrait handling of Utf8View and BinaryView (to avoid incurring more tech debt) -- but this code is not tested.

Are there any user-facing changes?

No API contract change.
Removal of unimplemented errors if using these new datatypes in subtrait serialization.

wiedld · 2024-08-27T18:44:36Z

datafusion/substrait/tests/cases/roundtrip_logical_plan.rs

+/// Arrow-cast does not currently handle direct casting from utf8 to binaryView.
+#[tokio::test]
+async fn binaryview_type_literal_needs_casting_fix() -> Result<()> {
+    let err = roundtrip_all_types(
+        "select * from data where
+            view_binary_col = arrow_cast('binary_view', 'BinaryView');",
+    )
+    .await;
+
+    assert!(
+        matches!(err, Err(e) if e.to_string().contains("Unsupported CAST from Utf8 to BinaryView"))
+    );
+    Ok(())


Looks like we have a few missing arrow_cast implementations for BinaryView (explicit casting). Going to file a ticket in arrow and put up a PR; I'll be assessing possible changes in cast_with_options and can_cast_types.

Note that datafusion's type coercion has been previously updated to prefer coercion to the view types. It's the explicit casting that has coverage gaps.

I see sqllogictests which demonstrate what is supported by arrow_cast. Then my follow ups will be: (a) make sqllogictests showing what is, and is not, supported of the new view types, and then (b) make the upstream arrow-rs changes (with some correctness guidance during code review).

Sqllogictests added: #12200

Turns out the arrow-cast changes are already made, but not in the current release used in datafusion.

We have now updated to the latest arrow-rs so we'll have the correct code #12032

Sweet. I've removed the work-arounds and deleted this (no longer applicable) test. Thank you.

wiedld · 2024-08-27T18:45:07Z

datafusion/substrait/tests/cases/roundtrip_logical_plan.rs

@@ -716,12 +716,29 @@ async fn all_type_literal() -> Result<()> {
            date32_col = arrow_cast('2020-01-01', 'Date32') AND
            binary_col = arrow_cast('binary', 'Binary') AND
            large_binary_col = arrow_cast('large_binary', 'LargeBinary') AND
+            view_binary_col = arrow_cast(arrow_cast('binary_view', 'Binary'), 'BinaryView') AND


See test binaryview_type_literal_needs_casting_fix() below, as for the reason behind the double casting.

I removed this workaround in a5bfedd

Blizzara · 2024-08-28T13:15:27Z

datafusion/substrait/src/variation_const.rs

@@ -52,6 +52,7 @@ pub const DATE_32_TYPE_VARIATION_REF: u32 = 0;
 pub const DATE_64_TYPE_VARIATION_REF: u32 = 1;
 pub const DEFAULT_CONTAINER_TYPE_VARIATION_REF: u32 = 0;
 pub const LARGE_CONTAINER_TYPE_VARIATION_REF: u32 = 1;
+pub const VIEW_CONTAINER_TYPE_VARIATION_REF: u32 = 2;


FWIW, hardcoding the numbers isn't really the proper way to do type variations. (Rather we should add the variation as an extension and refer to the extension's id.) However, given this is already used for default vs large, I guess adding view makes sense - and they can all be migrated at once to the proper way someday.

Thank you. I'll craft a follow up ticket later ~~today~~, and link here (for future reference).

I filed #12355 to track

…lize-views

alamb

Thank you @wiedld and @Blizzara -- I think this PR is looking good.

@wiedld now that we have updated to the latest arrow-rs (#12032) I updated this PR to remove the workarounds. I figured I had it checked out anyways so I would just push the change

alamb · 2024-09-05T20:18:51Z

datafusion/substrait/src/logical_plan/producer.rs

@@ -2351,8 +2371,10 @@ mod test {
        round_trip_type(DataType::Binary)?;
        round_trip_type(DataType::FixedSizeBinary(10))?;
        round_trip_type(DataType::LargeBinary)?;
+        round_trip_type(DataType::BinaryView)?;


alamb · 2024-09-05T20:20:05Z

datafusion/substrait/tests/cases/roundtrip_logical_plan.rs

+/// Arrow-cast does not currently handle direct casting from utf8 to binaryView.
+#[tokio::test]
+async fn binaryview_type_literal_needs_casting_fix() -> Result<()> {
+    let err = roundtrip_all_types(
+        "select * from data where
+            view_binary_col = arrow_cast('binary_view', 'BinaryView');",
+    )
+    .await;
+
+    assert!(
+        matches!(err, Err(e) if e.to_string().contains("Unsupported CAST from Utf8 to BinaryView"))
+    );
+    Ok(())


We have now updated to the latest arrow-rs so we'll have the correct code #12032

alamb · 2024-09-05T20:27:37Z

datafusion/substrait/tests/cases/roundtrip_logical_plan.rs

@@ -716,12 +716,29 @@ async fn all_type_literal() -> Result<()> {
            date32_col = arrow_cast('2020-01-01', 'Date32') AND
            binary_col = arrow_cast('binary', 'Binary') AND
            large_binary_col = arrow_cast('large_binary', 'LargeBinary') AND
+            view_binary_col = arrow_cast(arrow_cast('binary_view', 'Binary'), 'BinaryView') AND


I removed this workaround in a5bfedd

…row changes are in

alamb · 2024-09-06T11:10:43Z

Thanks again @wiedld and @Blizzara

wiedld added 2 commits August 27, 2024 10:19

feat(12118): logical plan support for Utf8View

d7be771

feat(12118): physical plan support for Utf8View

b17ae25

github-actions bot added the substrait label Aug 27, 2024

wiedld added 2 commits August 27, 2024 11:40

feat(12118): logical plan support for BinaryView

f38085d

feat(12118): physical plan support for BinaryView

5c4ebec

wiedld force-pushed the 12118/substrait-serialize-views branch from 700fe41 to 5c4ebec Compare August 27, 2024 18:41

wiedld commented Aug 27, 2024

View reviewed changes

wiedld marked this pull request as ready for review August 27, 2024 19:32

wiedld mentioned this pull request Aug 27, 2024

Define current arrow_cast behavior for BinaryView #12200

Merged

Blizzara reviewed Aug 28, 2024

View reviewed changes

Merge remote-tracking branch 'apache/main' into 12118/substrait-seria…

dc177d2

…lize-views

alamb approved these changes Sep 5, 2024

View reviewed changes

wiedld added 2 commits September 5, 2024 17:00

refactor(12118): remove BinaryView work-arounds, now that upstream ar…

831017c

…row changes are in

Merge branch 'main' into 12118/substrait-serialize-views

c587f99

alamb mentioned this pull request Sep 6, 2024

Type variation in substrait: Hardcoding the numbers isn't really the proper way to do type variations. #12355

Open

alamb merged commit aed84c2 into apache:main Sep 6, 2024
24 checks passed

alamb deleted the 12118/substrait-serialize-views branch September 6, 2024 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Utf8View and BinaryView in substrait serialization. #12199

Support Utf8View and BinaryView in substrait serialization. #12199

wiedld commented Aug 27, 2024 •

edited

Loading

wiedld Aug 27, 2024 •

edited

Loading

wiedld Aug 27, 2024 •

edited

Loading

wiedld Aug 27, 2024 •

edited

Loading

alamb Sep 5, 2024

wiedld Sep 6, 2024 •

edited

Loading

wiedld Aug 27, 2024

alamb Sep 5, 2024

Blizzara Aug 28, 2024

wiedld Aug 28, 2024 •

edited

Loading

alamb Sep 6, 2024

alamb left a comment

alamb Sep 5, 2024

alamb Sep 5, 2024

alamb Sep 5, 2024

alamb commented Sep 6, 2024

Support Utf8View and BinaryView in substrait serialization. #12199

Support Utf8View and BinaryView in substrait serialization. #12199

Conversation

wiedld commented Aug 27, 2024 • edited Loading

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?

wiedld Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

wiedld Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

wiedld Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiedld Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiedld Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Sep 6, 2024

wiedld commented Aug 27, 2024 •

edited

Loading

wiedld Aug 27, 2024 •

edited

Loading

wiedld Aug 27, 2024 •

edited

Loading

wiedld Aug 27, 2024 •

edited

Loading

wiedld Sep 6, 2024 •

edited

Loading

wiedld Aug 28, 2024 •

edited

Loading