feat: extend substrait type support, including type variations #5775

waynexia · 2023-03-29T12:43:11Z

Which issue does this PR close?

Closes #5717.

Rationale for this change

Extend the type and literal support in substrait. Most of simple type/literal are supported (exception: Date64 literal), and compound types except FixedSizeUtf8, FixedSizeList, Dictionary etc.

What changes are included in this PR?

Are these changes tested?

Some of, see comment below

Are there any user-facing changes?

Not breaking

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

waynexia

Hi @andygrove @nseekhao, please take a look when you have time, thanks!

There are some issues with testing and would greatly appreciate any advice you may have ❤️

waynexia · 2023-03-31T14:27:47Z

datafusion/substrait/tests/roundtrip_logical_plan.rs

+    // Literal in this cases have incorrect type. This is not a good case
+    #[tokio::test]
+    async fn other_type_literal() -> Result<()> {


Literals in this case have different types with the columns 😢

I think you can use the arrow_cast function to get specific types -- like arrow_cast('foo', LargeUtf8) for example

TIL, thanks for your help!

waynexia · 2023-03-31T14:31:44Z

datafusion/substrait/tests/roundtrip_logical_plan.rs

        Ok(())
    }

+    async fn roundtrip_all_types(sql: &str) -> Result<()> {


When I wrote cases like "select cast(a as boolean) from data" I got an error saying that there is no column a in the table (however "select a from data" works). I'm not sure if it's expected. But only cast uses type for now. Do you have any suggestions about testing new types?

Does the following syntax work?

select a::boolean from data work?

It works! I rewrite this case. But only a few SQL types are supported for now (I guess they are defined in this function).

I think the ::boolean syntax is in terms of SQL types (not Arrow types). The SQL --> Arrow type mapping are in

https://arrow.apache.org/datafusion/user-guide/sql/data_types.html

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

alamb

@andygrove / @waynexia is there someone who knows the datafusion-substrait crate that can help review this PR?

waynexia · 2023-04-05T03:22:17Z

@andygrove / @waynexia is there someone who knows the datafusion-substrait crate that can help review this PR?

cc @mbrobbel for who has contributed to this crate. I can also split this PR into two parts for types and literals to reduce the review burden.

alamb · 2023-04-05T12:48:19Z

If no one else reviews this I will find time to do so later today

alamb

Looks like an improvement to me -- thank you very much @waynexia

I am concerned about the use of unsafe / transmute but that was not introduced by this PR and thus I don't think it is needed to merge

alamb · 2023-04-06T17:52:29Z

datafusion/substrait/src/lib.rs

 pub mod logical_plan;
 pub mod physical_plan;
 pub mod serializer;
+mod variation_const;


The rest of these modules are pub but the newly added variation_const is not -- is that intentional?

Good catch!

alamb · 2023-04-06T17:56:50Z

datafusion/substrait/src/variation_const.rs

+
+//! Type variation constants
+
+pub const DEFAULT_TYPE_REF: u32 = 0;


I am not familiar with substrait's type variations --perhaps it would help to add a reference here to the subtrait documents that describe this and what they mean

Good idea. I add some documents and links to mod-level documentation.

this is very helpful

alamb · 2023-04-06T18:03:30Z

datafusion/substrait/src/logical_plan/consumer.rs

+        },
+        Some(LiteralType::I32(n)) => match lit.type_variation_reference {
+            DEFAULT_TYPE_REF => ScalarValue::Int32(Some(*n)),
+            UNSIGNED_INTEGER_TYPE_REF => ScalarValue::UInt32(Some(unsafe {


The use of unsafe here seems strange to me for two reasons:

It is inconsistent with Int16/UInt16 use n as i16 / n as u16

it doesn't seem necessary --it is perfectly safe to cast i32 to u32 -- for example https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=cfd7670d0675e2353f4f8c58616b3cec

I realize you just moved this code around, but I think it would be good to get rid of transmute

This comment applies to Int64 as well

it is perfectly safe to cast i32 to u32

This is because the "producer" (encoder) transmutes u32 to i32, which changes the actual number. E.g., when encoding an u32::MAX. Thus here we should also transmute back to get the original value.

These transmutations are dangerous and not straightforward. What do you think about replacing them with this cast method? Though it's also a transmute underlying, it would make the code looks more concrete like the following, and I believe those type annotations (::<u32,i32>) can be omitted.

// producer ScalarValue::UInt32(Some(n)) => LiteralType::I32(cast::<u32,i32>(n)}), // consumer UNSIGNED_INTEGER_TYPE_REF => ScalarValue::UInt32(Some(cast::<i32,u32>(n))),

I think I found a way to remove the unsafe code, see #5946

alamb · 2023-04-06T18:09:38Z

datafusion/substrait/tests/roundtrip_logical_plan.rs

+        let ctx = SessionContext::new();
+        let mut explicit_options = CsvReadOptions::new();
+        let schema = Schema::new(vec![
+            Field::new("a", DataType::Boolean, true),


I would personally find this test setup easier to use and understand if the fields were named for the type (for exmaple "bool_col" rather than "a"

The reason is that using just letters means that it is hard to understand what a particular test is testing without looking at this function

Thanks for the suggestion, it looks cleaner now 👍

alamb · 2023-04-06T18:10:02Z

datafusion/substrait/tests/roundtrip_logical_plan.rs

+            f = 0 AND
+            g = 0 AND
+            h = 0 AND
+            i = 0;",


I wonder if there is any reason for not testing j and k (f32 and f64) as well?

alamb · 2023-04-06T18:11:46Z

datafusion/substrait/tests/roundtrip_logical_plan.rs

+    // Literal in this cases have incorrect type. This is not a good case
+    #[tokio::test]
+    async fn other_type_literal() -> Result<()> {


I think you can use the arrow_cast function to get specific types -- like arrow_cast('foo', LargeUtf8) for example

alamb · 2023-04-06T18:12:31Z

datafusion/substrait/tests/roundtrip_logical_plan.rs

        Ok(())
    }

+    async fn roundtrip_all_types(sql: &str) -> Result<()> {


Does the following syntax work?

select a::boolean from data work?

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

waynexia

Thanks for reviewing this @alamb ❤️ I've pushed a new commit to solve comments, please take a look

waynexia · 2023-04-10T06:51:30Z

datafusion/substrait/src/lib.rs

 pub mod logical_plan;
 pub mod physical_plan;
 pub mod serializer;
+mod variation_const;


Good catch!

waynexia · 2023-04-10T07:08:22Z

datafusion/substrait/src/logical_plan/consumer.rs

+        },
+        Some(LiteralType::I32(n)) => match lit.type_variation_reference {
+            DEFAULT_TYPE_REF => ScalarValue::Int32(Some(*n)),
+            UNSIGNED_INTEGER_TYPE_REF => ScalarValue::UInt32(Some(unsafe {


it is perfectly safe to cast i32 to u32

This is because the "producer" (encoder) transmutes u32 to i32, which changes the actual number. E.g., when encoding an u32::MAX. Thus here we should also transmute back to get the original value.

These transmutations are dangerous and not straightforward. What do you think about replacing them with this cast method? Though it's also a transmute underlying, it would make the code looks more concrete like the following, and I believe those type annotations (::<u32,i32>) can be omitted.

// producer ScalarValue::UInt32(Some(n)) => LiteralType::I32(cast::<u32,i32>(n)}), // consumer UNSIGNED_INTEGER_TYPE_REF => ScalarValue::UInt32(Some(cast::<i32,u32>(n))),

waynexia · 2023-04-10T07:19:28Z

datafusion/substrait/src/variation_const.rs

+
+//! Type variation constants
+
+pub const DEFAULT_TYPE_REF: u32 = 0;


Good idea. I add some documents and links to mod-level documentation.

waynexia · 2023-04-10T07:28:43Z

datafusion/substrait/tests/roundtrip_logical_plan.rs

+        let ctx = SessionContext::new();
+        let mut explicit_options = CsvReadOptions::new();
+        let schema = Schema::new(vec![
+            Field::new("a", DataType::Boolean, true),


Thanks for the suggestion, it looks cleaner now 👍

waynexia · 2023-04-10T08:14:15Z

datafusion/substrait/tests/roundtrip_logical_plan.rs

+    // Literal in this cases have incorrect type. This is not a good case
+    #[tokio::test]
+    async fn other_type_literal() -> Result<()> {


TIL, thanks for your help!

waynexia · 2023-04-10T08:41:19Z

datafusion/substrait/tests/roundtrip_logical_plan.rs

        Ok(())
    }

+    async fn roundtrip_all_types(sql: &str) -> Result<()> {


It works! I rewrite this case. But only a few SQL types are supported for now (I guess they are defined in this function).

alamb · 2023-04-10T14:15:54Z

Thanks again @waynexia

…e#5775) * feat: extend types Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * feat: extend literal Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * fix clippy warnings Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * add some cases Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * clean up Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * add documentations and accomplish test Signed-off-by: Ruihang Xia <waynestxia@gmail.com> --------- Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

feat: extend types

643905d

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

github-actions bot added the substrait Changes to the substrait crate label Mar 29, 2023

feat: extend literal

4768925

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

waynexia marked this pull request as ready for review March 31, 2023 13:28

waynexia added 2 commits March 31, 2023 21:35

fix clippy warnings

d31df19

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

add some cases

5669d34

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

waynexia commented Mar 31, 2023

View reviewed changes

clean up

a0b32b1

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

alamb reviewed Apr 4, 2023

View reviewed changes

alamb changed the title ~~feat: extend substrait types~~ feat: extend substrait type support Apr 6, 2023

alamb approved these changes Apr 6, 2023

View reviewed changes

alamb changed the title ~~feat: extend substrait type support~~ feat: extend substrait type support, including type variations Apr 6, 2023

add documentations and accomplish test

f2b8db3

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

waynexia commented Apr 10, 2023

View reviewed changes

alamb merged commit 98197d5 into apache:main Apr 10, 2023

alamb mentioned this pull request Apr 10, 2023

Remove unsafe code (transmute) in datafusion-proto #5946

Merged

waynexia mentioned this pull request Apr 10, 2023

fix: match boolean null for substrait #5947

Merged

waynexia deleted the substrait-type branch April 11, 2023 02:58


		//! Type variation constants

		pub const DEFAULT_TYPE_REF: u32 = 0;

feat: extend substrait type support, including type variations #5775

feat: extend substrait type support, including type variations #5775

Uh oh!

Conversation

waynexia commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

waynexia commented Apr 5, 2023

Uh oh!

alamb commented Apr 5, 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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

waynexia commented Mar 29, 2023 •

edited

Loading