You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "tshauck (via GitHub)" <gi...@apache.org> on 2024/06/18 03:53:16 UTC
[PR] feat: extract floats for parquet data pages [datafusion]
tshauck opened a new pull request, #10972:
URL: https://github.com/apache/datafusion/pull/10972
## Which issue does this PR close?
Closes #10951
## Rationale for this change
Serves the overall epic https://github.com/apache/datafusion/issues/10922
## What changes are included in this PR?
1. Adds min and max DataPageStatsIterator for float32 and float64
2. Updates the main match in `get_data_page_statistics` to include f16, f32, and f64
3. Updates `null_counts_page_statistics` to handle floats
4. Updates various test utils to work with floats
## Are these changes tested?
Yes, added new tests.
## Are there any user-facing changes?
No
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1644954474
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
Oooff, sorry @tmi, I should've done `take` on the issue. Taking @alamb's pointer to look at row groups, I might be wrong, but I think there's a couple of differences between it and the data page stats that complicate things...
* `max_bytes`/`min_bytes`, which is used for row groups, does not exist on `PageIndex` in the parquet crate but does on `Statistics`, so using it in the macro isn't possible at this point.
* Using `[u8]` as the `stat_value_type` is problematic when combined with the iterator macro in that `type Item = Vec<Option<$stat_value_type>>;` can't work with `[u8]` because it's unsized. Changing the item def to `type Item = Vec<Option<&'a $stat_value_type>>;` works and is similar to the item definition of row group (`type Item = Option<&'a $stat_value_type>;`).
f16 is less common, so maybe to have a solution that's similar to Statistics and hopefully avoids cloning, we could:
1. Update this PR to do f32 and f64, get it in to support higher priority types and is mostly innocuous
2. Update the parquet crate to support `max_bytes`/`min_bytes` on `PageIndex`
3. Update the data page stats to use a `&'a $stat_value_type` and add f16 when that's merged
@tmi, maybe you and I could collaborate on 2 & 3 if that seems reasonable? Otherwise, certainly happy to go another route, if there's a better approach.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "tmi (via GitHub)" <gi...@apache.org>.
tmi commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1644665507
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
hi there, I haven't noticed this PR and accidentally addressed the same issue in here https://github.com/apache/datafusion/pull/10982 . Apologies. I think the f32 and f64 cases we solve the ~same. I got the f16 working, though not in a way I'm particularly happy with (ie, the move issues you mention)
I don't have any other commits in that PR in my queue -- so happy to close it if you come up with any other f16 solution...
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1643756240
##########
datafusion/core/tests/parquet/arrow_statistics.rs:
##########
@@ -507,9 +507,84 @@ async fn test_multiple_data_pages_nulls_and_negatives() {
/////////////// MORE GENERAL TESTS //////////////////////
// . Many columns in a file
-// . Differnet data types
+// . Different data types
// . Different row group sizes
+// Float 16, 32, 64
+#[tokio::test]
+async fn test_float_16() {
+ let reader = TestReader {
+ scenario: Scenario::Float16,
+ row_per_group: 5,
+ }
+ .build()
+ .await;
+
+ Test {
+ reader: &reader,
+ expected_min: Arc::new(Float16Array::from(
+ vec![-5.0, -4.0, -0.0, 5.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_max: Arc::new(Float16Array::from(
+ vec![-1.0, 0.0, 4.0, 9.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_null_counts: UInt64Array::from(vec![0, 0, 0, 0]),
+ expected_row_counts: Some(UInt64Array::from(vec![5, 5, 5, 5])),
+ column_name: "f",
+ check: Check::RowGroup,
Review Comment:
Thanks, I think you're right, but doing that leads to a panic 🤔 ... gonna move the PR back draft while I look further into it.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1645029524
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
Ok, sounds good to me. Maybe a further improvement down the line to avoid the copy and simplify to use a method vs a function. Shall I close this one?
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1643748991
##########
datafusion/core/tests/parquet/arrow_statistics.rs:
##########
@@ -507,9 +507,84 @@ async fn test_multiple_data_pages_nulls_and_negatives() {
/////////////// MORE GENERAL TESTS //////////////////////
// . Many columns in a file
-// . Differnet data types
+// . Different data types
Review Comment:
drive by cleanup 🚀
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1644954474
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
Oooff, sorry @tmi, I should've done `take` on the issue. Taking @alamb's pointer to look at row groups, I might be wrong, but I think there's a couple of differences between it and the data page stats that complicate things...
* `max_bytes`/`min_bytes`, which is used for row groups, does not exist on `PageIndex` in the parquet crate but does on `Statistics`, so using it in the macro isn't possible at this point.
* Using `[u8]` as the `stat_value_type` is problematic when combined with the iterator macro in that `type Item = Vec<Option<$stat_value_type>>;` can't work with `[u8]` because it's unsized. Changing the item def to `type Item = Vec<Option<&'a $stat_value_type>>;` works and is similar to the item definition of page stats (`type Item = Option<&'a $stat_value_type>;`).
f16 is less common, so maybe to have a solution that's similar to Statistics and hopefully avoids cloning, we could:
1. Update this PR to do f32 and f64, get it in to support higher priority types and is mostly innocuous
2. Update the parquet to support `max_bytes`/`min_bytes` on `PageIndex`
3. Update the data page stats to use a `&'a $stat_value_type` and add f16 when that's merged
@tmi, maybe you and I could collaborate on 2 & 3 if that seems reasonable? Otherwise, certainly happy to go another route, if there's a better approach.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1644556162
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
Apologies for the confusion. It pushed this as a WIP. I still need to work through the f16 case. I think I will need to construct an f16 from the bytes, but not sure yet.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1645029524
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
Ok, sounds good to me. Maybe a further improvement down the line to avoid the clone and simplify to use a method vs a function. I'll close this one.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1643749724
##########
datafusion/core/tests/parquet/arrow_statistics.rs:
##########
@@ -507,9 +507,84 @@ async fn test_multiple_data_pages_nulls_and_negatives() {
/////////////// MORE GENERAL TESTS //////////////////////
// . Many columns in a file
-// . Differnet data types
+// . Different data types
// . Different row group sizes
+// Float 16, 32, 64
+#[tokio::test]
+async fn test_float_16() {
+ let reader = TestReader {
+ scenario: Scenario::Float16,
+ row_per_group: 5,
+ }
+ .build()
+ .await;
+
+ Test {
+ reader: &reader,
+ expected_min: Arc::new(Float16Array::from(
+ vec![-5.0, -4.0, -0.0, 5.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_max: Arc::new(Float16Array::from(
+ vec![-1.0, 0.0, 4.0, 9.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_null_counts: UInt64Array::from(vec![0, 0, 0, 0]),
+ expected_row_counts: Some(UInt64Array::from(vec![5, 5, 5, 5])),
+ column_name: "f",
+ check: Check::RowGroup,
Review Comment:
I think this should be `Check::Both` ?
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1643863158
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -581,7 +597,20 @@ macro_rules! get_data_page_statistics {
)),
Some(DataType::Int32) => Ok(Arc::new(Int32Array::from_iter([<$stat_type_prefix Int32DataPageStatsIterator>]::new($iterator).flatten()))),
Some(DataType::Int64) => Ok(Arc::new(Int64Array::from_iter([<$stat_type_prefix Int64DataPageStatsIterator>]::new($iterator).flatten()))),
- _ => unimplemented!()
+ Some(DataType::Float16) => Ok(Arc::new(
+ Float16Array::from_iter(
+ [<$stat_type_prefix Float32DataPageStatsIterator>]::new($iterator)
+ .map(|x| {
+ x.into_iter().filter_map(|x| {
+ x.and_then(|x| Some(f16::from_f32(x)))
Review Comment:
@tshauck
I think you can mimic the approach [here](https://github.com/apache/datafusion/blob/766ee0480f73183a1ebc27b8243cfacbd7903c7c/datafusion/core/src/datasource/physical_plan/parquet/statistics.rs#L336-L338)? This might also fix the failing test.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1643891149
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
@tshauck
It seems like you are not using those in the maching down below? that might cause your error on f16. There you are using an Float32 iterator which won't match on Index::FIXED_LEN_BYTE_ARRAY and thus return None.
I think you need to switch to the f16 iterator; but perhaps might need to construct an f16 from the bytes.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1644588031
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
Here's an example of what I mean...
<img width="1130" alt="image" src="https://github.com/apache/datafusion/assets/421839/852f9b91-7b50-4903-8c59-cdd0bcd15fca">
Fixed length byte array is collected as `parquet::data_type::FixedLenByteArray`, which then means this doesn't match. Alternatively, using that data type in the macro causes later move issues when interacting with the calls to `min()` and `max()`.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1645027416
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
I think https://github.com/apache/datafusion/pull/10982 is good to go FWIW
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1645029524
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
Ok, sounds good to me. Maybe a further improvement down the line to avoid the copy and simplify to use a method vs a function. I'll close this one.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1643863158
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -581,7 +597,20 @@ macro_rules! get_data_page_statistics {
)),
Some(DataType::Int32) => Ok(Arc::new(Int32Array::from_iter([<$stat_type_prefix Int32DataPageStatsIterator>]::new($iterator).flatten()))),
Some(DataType::Int64) => Ok(Arc::new(Int64Array::from_iter([<$stat_type_prefix Int64DataPageStatsIterator>]::new($iterator).flatten()))),
- _ => unimplemented!()
+ Some(DataType::Float16) => Ok(Arc::new(
+ Float16Array::from_iter(
+ [<$stat_type_prefix Float32DataPageStatsIterator>]::new($iterator)
+ .map(|x| {
+ x.into_iter().filter_map(|x| {
+ x.and_then(|x| Some(f16::from_f32(x)))
Review Comment:
@tshauck
I think you can mimic the approach [here](https://github.com/apache/datafusion/blob/766ee0480f73183a1ebc27b8243cfacbd7903c7c/datafusion/core/src/datasource/physical_plan/parquet/statistics.rs#L336-L338)? This might also fix the failing test.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1643761958
##########
datafusion/core/tests/parquet/arrow_statistics.rs:
##########
@@ -507,9 +507,84 @@ async fn test_multiple_data_pages_nulls_and_negatives() {
/////////////// MORE GENERAL TESTS //////////////////////
// . Many columns in a file
-// . Differnet data types
+// . Different data types
// . Different row group sizes
+// Float 16, 32, 64
+#[tokio::test]
+async fn test_float_16() {
+ let reader = TestReader {
+ scenario: Scenario::Float16,
+ row_per_group: 5,
+ }
+ .build()
+ .await;
+
+ Test {
+ reader: &reader,
+ expected_min: Arc::new(Float16Array::from(
+ vec![-5.0, -4.0, -0.0, 5.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_max: Arc::new(Float16Array::from(
+ vec![-1.0, 0.0, 4.0, 9.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_null_counts: UInt64Array::from(vec![0, 0, 0, 0]),
+ expected_row_counts: Some(UInt64Array::from(vec![5, 5, 5, 5])),
+ column_name: "f",
+ check: Check::RowGroup,
Review Comment:
I think this is due to the difference in handling row_counts for RowGroups and DataPages. #10965
Im working on this - for now you'd need two test cases, one for each check with different expected row_counts.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1644659198
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
I suggest we take a friendly look at how f16 for row groups work: https://github.com/apache/datafusion/blob/1cb0057b99a016ee982d34deb860cf7f4e52b845/datafusion/core/src/datasource/physical_plan/parquet/statistics.rs#L336-L340
I suspect the statistics in data pages will be encodd the same
It is also possible that since f16 is a less common type, the parquet writer might write data page statistics for it
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1643756240
##########
datafusion/core/tests/parquet/arrow_statistics.rs:
##########
@@ -507,9 +507,84 @@ async fn test_multiple_data_pages_nulls_and_negatives() {
/////////////// MORE GENERAL TESTS //////////////////////
// . Many columns in a file
-// . Differnet data types
+// . Different data types
// . Different row group sizes
+// Float 16, 32, 64
+#[tokio::test]
+async fn test_float_16() {
+ let reader = TestReader {
+ scenario: Scenario::Float16,
+ row_per_group: 5,
+ }
+ .build()
+ .await;
+
+ Test {
+ reader: &reader,
+ expected_min: Arc::new(Float16Array::from(
+ vec![-5.0, -4.0, -0.0, 5.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_max: Arc::new(Float16Array::from(
+ vec![-1.0, 0.0, 4.0, 9.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_null_counts: UInt64Array::from(vec![0, 0, 0, 0]),
+ expected_row_counts: Some(UInt64Array::from(vec![5, 5, 5, 5])),
+ column_name: "f",
+ check: Check::RowGroup,
Review Comment:
Thanks, I think you're right, but doing that leads to a test assertion failure 🤔 ... gonna move the PR back draft while I look further into it.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1644954474
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
Oooff, sorry @tmi, I should've done `take` on the issue. Taking @alamb's pointer to look at row groups, I might be wrong, but I think there's a couple of differences between it and the data page stats that complicate things...
* `max_bytes`/`min_bytes`, which is used for row groups, does not exist on `PageIndex` in the parquet crate but does on `Statistics`, so using it in the macro isn't possible at this point.
* Using `[u8]` as the `stat_value_type` is problematic when combined with the iterator macro in that `type Item = Vec<Option<$stat_value_type>>;` can't work with `[u8]` because it's unsized. Changing the item def to `type Item = Vec<Option<&'a $stat_value_type>>;` works and is similar to the item definition of row group (`type Item = Option<&'a $stat_value_type>;`).
f16 is less common, so maybe to have a solution that's similar to Statistics and hopefully avoids cloning, we could:
1. Update this PR to do f32 and f64, get it in to support higher priority types and is mostly innocuous
2. Update the parquet to support `max_bytes`/`min_bytes` on `PageIndex`
3. Update the data page stats to use a `&'a $stat_value_type` and add f16 when that's merged
@tmi, maybe you and I could collaborate on 2 & 3 if that seems reasonable? Otherwise, certainly happy to go another route, if there's a better approach.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1643891149
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
@tshauck
It seems like you are not using those in the maching down below? that might cause your error on f16. Instead you are using an Float32 iterator which won't match on Index::FIXED_LEN_BYTE_ARRAY and thus return None.
I think you need to switch to the f16 iterator; but perhaps might need to construct an f16 from the bytes.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1644655618
##########
datafusion/core/tests/parquet/arrow_statistics.rs:
##########
@@ -507,9 +507,84 @@ async fn test_multiple_data_pages_nulls_and_negatives() {
/////////////// MORE GENERAL TESTS //////////////////////
// . Many columns in a file
-// . Differnet data types
+// . Different data types
// . Different row group sizes
+// Float 16, 32, 64
+#[tokio::test]
+async fn test_float_16() {
+ let reader = TestReader {
+ scenario: Scenario::Float16,
+ row_per_group: 5,
+ }
+ .build()
+ .await;
+
+ Test {
+ reader: &reader,
+ expected_min: Arc::new(Float16Array::from(
+ vec![-5.0, -4.0, -0.0, 5.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_max: Arc::new(Float16Array::from(
+ vec![-1.0, 0.0, 4.0, 9.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_null_counts: UInt64Array::from(vec![0, 0, 0, 0]),
+ expected_row_counts: Some(UInt64Array::from(vec![5, 5, 5, 5])),
+ column_name: "f",
+ check: Check::RowGroup,
Review Comment:
FWIW https://github.com/apache/datafusion/issues/10965 is now fixex
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1644988813
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
Review Comment:
Sounds like a good plan to me
I think it is perfectly reasonable to leave `f16` support as a 'TODO' in datafusion as we add the needed support upstream
Let me know if I can help (e.g. file some tickets, etc
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1643868408
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -552,6 +552,22 @@ make_data_page_stats_iterator!(MinInt32DataPageStatsIterator, min, Index::INT32,
make_data_page_stats_iterator!(MaxInt32DataPageStatsIterator, max, Index::INT32, i32);
make_data_page_stats_iterator!(MinInt64DataPageStatsIterator, min, Index::INT64, i64);
make_data_page_stats_iterator!(MaxInt64DataPageStatsIterator, max, Index::INT64, i64);
+make_data_page_stats_iterator!(
+ MinFloat16DataPageStatsIterator,
+ min,
+ Index::FIXED_LEN_BYTE_ARRAY,
+ f16
+);
+make_data_page_stats_iterator!(
+ MaxFloat16DataPageStatsIterator,
+ max,
+ Index::FIXED_LEN_BYTE_ARRAY,
+ f16
+);
+make_data_page_stats_iterator!(MinFloat32DataPageStatsIterator, min, Index::FLOAT, f32);
Review Comment:
I think `Float` (f32) and `Double` (f64) would be enough, in terms of naming. This would also be consistent with the row group stats iterators.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck closed pull request #10972: feat: extract floats for parquet data pages
URL: https://github.com/apache/datafusion/pull/10972
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1643805987
##########
datafusion/core/tests/parquet/arrow_statistics.rs:
##########
@@ -507,9 +507,84 @@ async fn test_multiple_data_pages_nulls_and_negatives() {
/////////////// MORE GENERAL TESTS //////////////////////
// . Many columns in a file
-// . Differnet data types
+// . Different data types
// . Different row group sizes
+// Float 16, 32, 64
+#[tokio::test]
+async fn test_float_16() {
+ let reader = TestReader {
+ scenario: Scenario::Float16,
+ row_per_group: 5,
+ }
+ .build()
+ .await;
+
+ Test {
+ reader: &reader,
+ expected_min: Arc::new(Float16Array::from(
+ vec![-5.0, -4.0, -0.0, 5.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_max: Arc::new(Float16Array::from(
+ vec![-1.0, 0.0, 4.0, 9.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_null_counts: UInt64Array::from(vec![0, 0, 0, 0]),
+ expected_row_counts: Some(UInt64Array::from(vec![5, 5, 5, 5])),
+ column_name: "f",
+ check: Check::RowGroup,
Review Comment:
I'm stretching a here, but doing a bit of looking I think it's due to the physical type for an f16 being a fixed length byte array. I think `column_page_index_per_row_group_per_column` is an `Index::FIXED_LEN_BYTE_ARRAY` which seemingly isn't playing well with some of the conversion, so the values coming through end up being None.
--
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] feat: extract floats for parquet data pages [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #10972:
URL: https://github.com/apache/datafusion/pull/10972#discussion_r1643892813
##########
datafusion/core/tests/parquet/arrow_statistics.rs:
##########
@@ -507,9 +507,84 @@ async fn test_multiple_data_pages_nulls_and_negatives() {
/////////////// MORE GENERAL TESTS //////////////////////
// . Many columns in a file
-// . Differnet data types
+// . Different data types
// . Different row group sizes
+// Float 16, 32, 64
+#[tokio::test]
+async fn test_float_16() {
+ let reader = TestReader {
+ scenario: Scenario::Float16,
+ row_per_group: 5,
+ }
+ .build()
+ .await;
+
+ Test {
+ reader: &reader,
+ expected_min: Arc::new(Float16Array::from(
+ vec![-5.0, -4.0, -0.0, 5.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_max: Arc::new(Float16Array::from(
+ vec![-1.0, 0.0, 4.0, 9.0]
+ .into_iter()
+ .map(f16::from_f32)
+ .collect::<Vec<_>>(),
+ )),
+ expected_null_counts: UInt64Array::from(vec![0, 0, 0, 0]),
+ expected_row_counts: Some(UInt64Array::from(vec![5, 5, 5, 5])),
+ column_name: "f",
+ check: Check::RowGroup,
Review Comment:
you seem to be right here; I left a comment in the code - where I think you might have missed something.
--
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