You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "wiedld (via GitHub)" <gi...@apache.org> on 2024/08/27 18:04:26 UTC
[PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
wiedld opened a new pull request, #12199:
URL: https://github.com/apache/datafusion/pull/12199
**Not ready for review. Demonstrating CI failure, then pushing next commit which fixes the casting.**
## Which issue does this PR close?
Closes #12118
## Rationale for this change
We have two new view data types, [Utf8View](https://docs.rs/arrow-schema/52.2.0/arrow_schema/enum.DataType.html#variant.Utf8View) and [BinaryView](https://docs.rs/arrow-schema/52.2.0/arrow_schema/enum.DataType.html#variant.BinaryView). Support in datafusion is part of [this epic](https://github.com/apache/datafusion/issues/11752), 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](https://github.com/apache/datafusion/issues/12118#issuecomment-2311396274).
## What changes are included in this PR?
* feat(12118): logical plan support for Utf8View (d7be771eb)
* feat(12118): physical plan support for Utf8View (b17ae25a7)
* feat(12118): logical plan support for BinaryView (8ca5fc147)
* feat(12118): physical plan support for BinaryView (700fe4155)
* TODO -- last commit which fixes the cast handling for binary view. See CI test failure (demonstrating need) first.
*
## Are these changes tested?
The Utf8View and BinaryView are covered in the logical plan roundtrip serialization tests.
However, the physical plan roundtrip serialization tests are not yet implement. There is an [ongoing epic](https://github.com/apache/datafusion/issues/5173) to finish the physical plan serialization. As such, I added 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1733470461
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
Sqllogictests added: https://github.com/apache/datafusion/pull/12200
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1733416035
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
I [see sqllogictests](https://github.com/apache/datafusion/blob/6ffb1f64a7b3f48c7693d2ecb34f72a2dca24acc/datafusion/sqllogictest/test_files/arrow_typeof.slt#L316) 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).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#issuecomment-2333824256
Thanks again @wiedld and @Blizzara
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "Blizzara (via GitHub)" <gi...@apache.org>.
Blizzara commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1734657314
##########
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;
Review Comment:
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1746111510
##########
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)?;
Review Comment:
👍
##########
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
Review Comment:
I removed this workaround in a5bfedd0d
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
We have now updated to the latest arrow-rs so we'll have the correct code https://github.com/apache/datafusion/pull/12032
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #12199:
URL: https://github.com/apache/datafusion/pull/12199
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1735038844
##########
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;
Review Comment:
Thank you. I'll craft a follow up ticket later ~today~, and link here (for future reference).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1733357307
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
Looks like we have a few other missing casts for BinaryView. Going to file a ticket and put up a PR in arrow-cast.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1733416035
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
I [see sqllogictests](https://github.com/apache/datafusion/blob/6ffb1f64a7b3f48c7693d2ecb34f72a2dca24acc/datafusion/sqllogictest/test_files/arrow_typeof.slt#L316) which demonstrate what is supported by arrow_cast (in addition to CAST tests). 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).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1733470461
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
Sqllogictests added: https://github.com/apache/datafusion/pull/12200
Turns out the [arrow-cast changes are already made](https://github.com/apache/arrow-rs/commit/4bd737dab2aa17aca200259347909d48ed793ba1), but not in the current release used in datafusion.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1733416035
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
I [see sqllogictests](https://github.com/apache/datafusion/blob/6ffb1f64a7b3f48c7693d2ecb34f72a2dca24acc/datafusion/sqllogictest/test_files/arrow_typeof.slt#L316) which demonstrate what is supported by arrow_cast (in addition to cast tests). 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).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1733357307
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
Looks like we have a few missing `arrow_cast` for BinaryView. Going to file a ticket in arrow and put up a PR.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1733357307
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
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](https://github.com/apache/arrow-rs/blob/8a66174d2b87d71d0ecc71b04b2ffcecfd9c1cbf/arrow-cast/src/cast/mod.rs#L678) and [can_cast_types](https://github.com/apache/arrow-rs/blob/8a66174d2b87d71d0ecc71b04b2ffcecfd9c1cbf/arrow-cast/src/cast/mod.rs#L92).
Note that this is different than datafusion's type coercion, which was [previously updated](https://github.com/apache/datafusion/commit/ccb4baf0fc6b4dee983bb29f2282b9c19510a481) to prefer coercion to the view types.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1746341501
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
Sweet. I've removed the work-arounds and deleted this (no longer applicable) test. Thank you.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1733416035
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
I [see sqllogictests](https://github.com/apache/datafusion/blob/6ffb1f64a7b3f48c7693d2ecb34f72a2dca24acc/datafusion/sqllogictest/test_files/arrow_typeof.slt#L316) 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).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1733357307
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
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](https://github.com/apache/arrow-rs/blob/8a66174d2b87d71d0ecc71b04b2ffcecfd9c1cbf/arrow-cast/src/cast/mod.rs#L678) and [can_cast_types](https://github.com/apache/arrow-rs/blob/8a66174d2b87d71d0ecc71b04b2ffcecfd9c1cbf/arrow-cast/src/cast/mod.rs#L92).
Note that datafusion's type coercion has been [previously updated](https://github.com/apache/datafusion/commit/ccb4baf0fc6b4dee983bb29f2282b9c19510a481) to prefer coercion to the view types. It's the explicit casting that has coverage gaps.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1735038844
##########
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;
Review Comment:
Thank you. I'll craft up a follow up ticket later today, and link here (for future reference).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1746341501
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
Sweet. I've removed the work-arounds. Thank you.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1746945928
##########
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;
Review Comment:
I filed https://github.com/apache/datafusion/issues/12355 to track
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1733357307
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
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](https://github.com/apache/arrow-rs/blob/8a66174d2b87d71d0ecc71b04b2ffcecfd9c1cbf/arrow-cast/src/cast/mod.rs#L678) and [can_cast_types](https://github.com/apache/arrow-rs/blob/8a66174d2b87d71d0ecc71b04b2ffcecfd9c1cbf/arrow-cast/src/cast/mod.rs#L92).
Note that this is different than datafusion's type coercion, that was [previously updated](https://github.com/apache/datafusion/commit/ccb4baf0fc6b4dee983bb29f2282b9c19510a481) to prefer coercion to the view types.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1733357307
##########
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
utf8_col = arrow_cast('utf8', 'Utf8') AND
- large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8');",
+ large_utf8_col = arrow_cast('large_utf8', 'LargeUtf8') AND
+ view_utf8_col = arrow_cast('utf8_view', 'Utf8View');",
)
.await
}
+/// 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(())
Review Comment:
Looks like we have a few other missing casts for BinaryView. Going to file a ticket and put up a PR.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1733357867
##########
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
Review Comment:
See test `binaryview_type_literal_needs_casting_fix()` below, as for the reason behind the double casting.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support Utf8View and BinaryView in substrait serialization. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12199:
URL: https://github.com/apache/datafusion/pull/12199#discussion_r1735038844
##########
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;
Review Comment:
Thank you. I'll craft a follow up ticket later today, and link here (for future reference).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org