You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kylebarron (via GitHub)" <gi...@apache.org> on 2024/12/16 17:14:35 UTC
[PR] Fix `ScalarValue::to_array_of_size` for DenseUnion [datafusion]
kylebarron opened a new pull request, #13797:
URL: https://github.com/apache/datafusion/pull/13797
## Which issue does this PR close?
Closes https://github.com/apache/datafusion/issues/13762
## Rationale for this change
Fix support for dense union.
## What changes are included in this PR?
Fix `ScalarValue::to_array_of_size` for DenseUnion
## Are these changes tested?
Yes. The test from https://github.com/apache/datafusion/pull/13777 is uncommented.
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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] Fix `ScalarValue::to_array_of_size` for DenseUnion [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #13797:
URL: https://github.com/apache/datafusion/pull/13797
--
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] Fix `ScalarValue::to_array_of_size` for DenseUnion [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13797:
URL: https://github.com/apache/datafusion/pull/13797#discussion_r1887328176
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -5642,14 +5650,12 @@ mod tests {
// null array
Arc::new(NullArray::new(3)),
// dense union
- /* Dense union fails due to https://github.com/apache/datafusion/issues/13762
{
Review Comment:
🎉
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -2453,15 +2453,23 @@ impl ScalarValue {
value.to_array_of_size(size)?
} else {
let dt = field.data_type();
- new_null_array(dt, size)
+ match mode {
+ UnionMode::Sparse => new_null_array(dt, size),
+ // In a dense union, only the child with values needs to be
Review Comment:
I double checked the spec and I agree with this check: https://arrow.apache.org/docs/format/Columnar.html#dense-union
--
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] Fix `ScalarValue::to_array_of_size` for DenseUnion [datafusion]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on PR #13797:
URL: https://github.com/apache/datafusion/pull/13797#issuecomment-2546221849
I also validated in https://github.com/geoarrow/geoarrow-rs/pull/951 that this is now working for storing geospatial geometries in a dense union!
Mirroring the [PostGIS `ST_GeomFromText`](https://postgis.net/docs/ST_GeomFromText.html):
```
SELECT ST_GeomFromText('LINESTRING(-71.160281 42.258729,-71.160837 42.259113,-71.161144 42.25932)');
```
```
---- udf::native::io::wkt::test::test stdout ----
+----------------------------------------------------------------------------------------------------+
| st_geomfromtext(Utf8("LINESTRING(-71.160281 42.258729,-71.160837 42.259113,-71.161144 42.25932)")) |
+----------------------------------------------------------------------------------------------------+
| {=[{x: -71.160281, y: 42.258729}, {x: -71.160837, y: 42.259113}, {x: -71.161144, y: 42.25932}]} |
+----------------------------------------------------------------------------------------------------+
```
Under the hood this is stored as a [rather complex dense union](https://github.com/geoarrow/geoarrow/blob/main/format.md#geometry), but allows for any combination of geometry type and dimension with a unique DataType that's streaming-friendly.
--
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