feat: extract floats for parquet data pages #10972

tshauck · 2024-06-18T03:53:14Z

Which issue does this PR close?

Closes #10951

Rationale for this change

Serves the overall epic #10922

What changes are included in this PR?

Adds min and max DataPageStatsIterator for float32 and float64
Updates the main match in get_data_page_statistics to include f16, f32, and f64
Updates null_counts_page_statistics to handle floats
Updates various test utils to work with floats

Are these changes tested?

Yes, added new tests.

Are there any user-facing changes?

No

marvinlanhenke · 2024-06-18T04:26:45Z

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


drive by cleanup 🚀

marvinlanhenke

@tshauck
Thanks LGTM. I think only one test case should check both instead of only row_groups?

marvinlanhenke · 2024-06-18T04:27:39Z

datafusion/core/tests/parquet/arrow_statistics.rs

+        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,


I think this should be Check::Both ?

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.

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.

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.

you seem to be right here; I left a comment in the code - where I think you might have missed something.

FWIW #10965 is now fixex

marvinlanhenke · 2024-06-18T06:11:34Z

datafusion/core/src/datasource/physical_plan/parquet/statistics.rs

+    Index::FIXED_LEN_BYTE_ARRAY,
+    f16
+);
+make_data_page_stats_iterator!(MinFloat32DataPageStatsIterator, min, Index::FLOAT, f32);


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.

marvinlanhenke · 2024-06-18T06:26:43Z

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!(


@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.

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.

Here's an example of what I mean...

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().

I suggest we take a friendly look at how f16 for row groups work:

datafusion/datafusion/core/src/datasource/physical_plan/parquet/statistics.rs

Lines 336 to 340 in 1cb0057

DataType::Float16 => Ok(Arc::new(Float16Array::from_iter(

[<$stat_type_prefix FixedLenByteArrayStatsIterator>]::new($iterator).map(|x| x.and_then(|x| {

from_bytes_to_f16(x)

})),

))),

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

hi there, I haven't noticed this PR and accidentally addressed the same issue in here #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...

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:

Update this PR to do f32 and f64, get it in to support higher priority types and is mostly innocuous

Update the parquet crate to support max_bytes/min_bytes on PageIndex

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.

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

I think #10982 is good to go FWIW

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.

avoid the clone

I think the ideal sequence would be

Tracking Issue for f16 and f128 float types rust-lang/rust#116909 gets merged

parquet replaces FixedLengthByteArray with genuine f16 type

we drop the special handling here and elsewhere

and until then the f16 support in datafusion would be as is... Or alternatively, introduce the half crate to parquet rs first. Because dealing with FixedLengthByteArray on the user side is always going to be messy

tshauck added 3 commits June 17, 2024 20:20

feat: add f32 stats extraction

0c39d51

feat: add f64

2f22012

feat: update

a44f973

github-actions bot added the core Core DataFusion crate label Jun 18, 2024

tshauck marked this pull request as ready for review June 18, 2024 04:19

marvinlanhenke reviewed Jun 18, 2024

View reviewed changes

tshauck marked this pull request as draft June 18, 2024 04:35

refactor: update tests; look at f16 type

766ee04

marvinlanhenke reviewed Jun 18, 2024

View reviewed changes

alamb mentioned this pull request Jun 18, 2024

Add parquet page stats for float{16, 32, 64} #10982

Merged

tshauck closed this Jun 18, 2024

tshauck deleted the extract-floats branch June 24, 2024 19:46

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: extract floats for parquet data pages #10972

feat: extract floats for parquet data pages #10972

tshauck commented Jun 18, 2024

marvinlanhenke Jun 18, 2024

marvinlanhenke left a comment

marvinlanhenke Jun 18, 2024

tshauck Jun 18, 2024 •

edited

Loading

marvinlanhenke Jun 18, 2024

tshauck Jun 18, 2024

marvinlanhenke Jun 18, 2024

alamb Jun 18, 2024

marvinlanhenke Jun 18, 2024

marvinlanhenke Jun 18, 2024 •

edited

Loading

tshauck Jun 18, 2024

tshauck Jun 18, 2024

alamb Jun 18, 2024

tmi Jun 18, 2024

tshauck Jun 18, 2024 •

edited

Loading

alamb Jun 18, 2024

alamb Jun 18, 2024

tshauck Jun 18, 2024 •

edited

Loading

tmi Jun 19, 2024

	DataType::Float16 => Ok(Arc::new(Float16Array::from_iter(
	[<$stat_type_prefix FixedLenByteArrayStatsIterator>]::new($iterator).map(\|x\| x.and_then(\|x\| {
	from_bytes_to_f16(x)
	})),
	))),

feat: extract floats for parquet data pages #10972

feat: extract floats for parquet data pages #10972

Conversation

tshauck commented Jun 18, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

marvinlanhenke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshauck Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marvinlanhenke Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshauck Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshauck Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshauck Jun 18, 2024 •

edited

Loading

marvinlanhenke Jun 18, 2024 •

edited

Loading

tshauck Jun 18, 2024 •

edited

Loading

tshauck Jun 18, 2024 •

edited

Loading