You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "etseidl (via GitHub)" <gi...@apache.org> on 2025/08/20 19:57:36 UTC
[PR] [thrift-remodel] Add custom `PageLocation` decoder for to speed up decoding of page indexes [arrow-rs]
etseidl opened a new pull request, #8190:
URL: https://github.com/apache/arrow-rs/pull/8190
# Which issue does this PR close?
**Note: this targets a feature branch, not main**
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
- Part of #5854.
# Rationale for this change
Add a custom parser for `PageLocation` as the decoding of this struct is one of several hot spots.
# What changes are included in this PR?
This adds a faster means of obtaining the struct field ids to `ThriftCompactInputProtocol`. For a small struct (3 fields) with all of them required, we can save a good bit of time bypassing `ThriftCompactInputProtocol::read_field_begin` which is very general and can handle out-of-order fields, among other things. By adding a new function `read_field_header`, we can avoid the costly branching that occurs when calculating the new field id (as well as special handling needed for boolean fields). Field validation is then handled on the consuming side while decoding the `PageLocation` struct.
Note that to obtain the speed up seen, we need to assume the fields will always be in order, and the field ids will all be encoded as field deltas. This is probably a fairly safe assumption, but there does exist the possibility of custom thrift writers that use absolute field ids. If we encounter such a writer in the wild, this change will need to be reverted.
# Are these changes tested?
These changes should be covered by existing changes.
# Are there any user-facing changes?
None beyond the changes in this branch.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Add custom `PageLocation` decoder to speed up decoding of page indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8190:
URL: https://github.com/apache/arrow-rs/pull/8190#issuecomment-3229530865
Going to commit this and move on to writes, etc. We can always revert this later if we don't like 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Add custom `PageLocation` decoder to speed up decoding of page indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl merged PR #8190:
URL: https://github.com/apache/arrow-rs/pull/8190
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Add custom `PageLocation` decoder to speed up decoding of page indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8190:
URL: https://github.com/apache/arrow-rs/pull/8190#discussion_r2296076837
##########
parquet/src/file/page_index/offset_index.rs:
##########
@@ -37,7 +37,67 @@ pub struct PageLocation {
/// (repetition_level = 0).
3: required i64 first_row_index
}
-);
+);*/
+
+// hand coding this one because it is very time critical
+
+/// Page location information for [`OffsetIndexMetaData`]
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct PageLocation {
+ /// Offset of the page in the file
+ pub offset: i64,
+ /// Size of the page, including header. Sum of compressed_page_size and header
+ pub compressed_page_size: i32,
+ /// Index within the RowGroup of the first row of the page. When an
+ /// OffsetIndex is present, pages must begin on row boundaries
+ /// (repetition_level = 0).
+ pub first_row_index: i64,
+}
+
+// Note: this will fail if the fields are either out of order, or if a suboptimal
+// encoder doesn't use field deltas. If that ever occurs, remove this code and
+// revert to the commented out thrift_struct!() implementation above.
+impl<'a> TryFrom<&mut ThriftCompactInputProtocol<'a>> for PageLocation {
+ type Error = ParquetError;
+ fn try_from(prot: &mut ThriftCompactInputProtocol<'a>) -> Result<Self> {
+ // there are 3 fields, all mandatory, so all field deltas should be 1
+ let (field_type, delta) = prot.read_field_header()?;
+ if delta != 1 || field_type != FieldType::I64 as u8 {
+ return Err(general_err!("error reading PageLocation::offset"));
+ }
+ let offset = prot.read_i64()?;
+
+ let (field_type, delta) = prot.read_field_header()?;
+ if delta != 1 || field_type != FieldType::I32 as u8 {
+ return Err(general_err!(
+ "error reading PageLocation::compressed_page_size"
+ ));
+ }
+ let compressed_page_size = prot.read_i32()?;
+
+ let (field_type, delta) = prot.read_field_header()?;
+ if delta != 1 || field_type != FieldType::I64 as u8 {
+ return Err(general_err!("error reading PageLocation::first_row_index"));
+ }
+ let first_row_index = prot.read_i64()?;
+
+ // This loop slows things down a bit, but it's an acceptible price to allow
+ // forwards compatibility. We could instead assert the next field is Stop.
Review Comment:
This pr definitely needs more benchmarking to justify it. On a xeon the speed up isn't as dramatic, so i might shelve this one for now. I do like the idea of keeping both paths, especially with the push decoder on the horizon.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Add custom `PageLocation` decoder for to speed up decoding of page indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8190:
URL: https://github.com/apache/arrow-rs/pull/8190#issuecomment-3207935254
From #8160 benchmark results (old is the original thrift, new is the reworked decoder)
```
old
open(page index) time: [1.7446 ms 1.7524 ms 1.7639 ms]
new
open(page index) time: [698.00 µs 699.75 µs 701.66 µs]
```
With these changes the reading of the page index is now:
```
open(page index) time: [613.47 µs 614.98 µs 616.68 µs]
```
Note that the decoding of the column index is ~2/3 of the decoding time, so the speed up in the offset index decoding is considerable.
Is this worth the added complexity is the question. No changes to the `PageLocation` struct are on the horizon, so maybe.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Add custom `PageLocation` decoder to speed up decoding of page indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8190:
URL: https://github.com/apache/arrow-rs/pull/8190#issuecomment-3224205460
> > .now the benchmark is down to 584us.
>
> Given the original description, does that mean that the orignal time was 1.75 ms (thus this PR is like a 3X improvement)?
>
> > open(page index) time: [1.7446 ms 1.7524 ms 1.7639 ms]
The 1.75ms is the time for the decoder currently in main. The first phase of the remodel got that down to 700us, and this PR gets the time down to 585us (so a further 16% improvement). When combined with the column index changes in #8191 this benchmark is down to under 290us.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Add custom `PageLocation` decoder to speed up decoding of page indexes [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8190:
URL: https://github.com/apache/arrow-rs/pull/8190#issuecomment-3223560943
> .now the benchmark is down to 584us.
Given the original description, does that mean that the orignal time was 1.75 ms (thus this PR is like a 3X improvement)?
> open(page index) time: [1.7446 ms 1.7524 ms 1.7639 ms]
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Add custom `PageLocation` decoder to speed up decoding of page indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8190:
URL: https://github.com/apache/arrow-rs/pull/8190#issuecomment-3217505316
I still need to test this more, but I followed your suggestion to fall back on error, and along the way did a custom implementation of all of `OffsetIndexMetaData`...now the benchmark is down to 584us.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Add custom `PageLocation` decoder to speed up decoding of page indexes [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8190:
URL: https://github.com/apache/arrow-rs/pull/8190#issuecomment-3258409085
> When combined with the column index changes in https://github.com/apache/arrow-rs/pull/8191 this benchmark is down to under 290us.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Add custom `PageLocation` decoder to speed up decoding of page indexes [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8190:
URL: https://github.com/apache/arrow-rs/pull/8190#discussion_r2295754079
##########
parquet/src/file/page_index/offset_index.rs:
##########
@@ -37,7 +37,67 @@ pub struct PageLocation {
/// (repetition_level = 0).
3: required i64 first_row_index
}
-);
+);*/
+
+// hand coding this one because it is very time critical
+
+/// Page location information for [`OffsetIndexMetaData`]
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct PageLocation {
+ /// Offset of the page in the file
+ pub offset: i64,
+ /// Size of the page, including header. Sum of compressed_page_size and header
+ pub compressed_page_size: i32,
+ /// Index within the RowGroup of the first row of the page. When an
+ /// OffsetIndex is present, pages must begin on row boundaries
+ /// (repetition_level = 0).
+ pub first_row_index: i64,
+}
+
+// Note: this will fail if the fields are either out of order, or if a suboptimal
Review Comment:
Another alternative might be to have a fallback, so that if we encountered an out of order field, it could discard the work so far, and fall back to the more general, but slower implementation 🤔
##########
parquet/src/file/page_index/offset_index.rs:
##########
@@ -37,7 +37,67 @@ pub struct PageLocation {
/// (repetition_level = 0).
3: required i64 first_row_index
}
-);
+);*/
+
+// hand coding this one because it is very time critical
+
+/// Page location information for [`OffsetIndexMetaData`]
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct PageLocation {
+ /// Offset of the page in the file
+ pub offset: i64,
+ /// Size of the page, including header. Sum of compressed_page_size and header
+ pub compressed_page_size: i32,
+ /// Index within the RowGroup of the first row of the page. When an
+ /// OffsetIndex is present, pages must begin on row boundaries
+ /// (repetition_level = 0).
+ pub first_row_index: i64,
+}
+
+// Note: this will fail if the fields are either out of order, or if a suboptimal
+// encoder doesn't use field deltas. If that ever occurs, remove this code and
+// revert to the commented out thrift_struct!() implementation above.
+impl<'a> TryFrom<&mut ThriftCompactInputProtocol<'a>> for PageLocation {
+ type Error = ParquetError;
+ fn try_from(prot: &mut ThriftCompactInputProtocol<'a>) -> Result<Self> {
+ // there are 3 fields, all mandatory, so all field deltas should be 1
+ let (field_type, delta) = prot.read_field_header()?;
+ if delta != 1 || field_type != FieldType::I64 as u8 {
+ return Err(general_err!("error reading PageLocation::offset"));
+ }
+ let offset = prot.read_i64()?;
+
+ let (field_type, delta) = prot.read_field_header()?;
+ if delta != 1 || field_type != FieldType::I32 as u8 {
+ return Err(general_err!(
+ "error reading PageLocation::compressed_page_size"
+ ));
+ }
+ let compressed_page_size = prot.read_i32()?;
+
+ let (field_type, delta) = prot.read_field_header()?;
+ if delta != 1 || field_type != FieldType::I64 as u8 {
+ return Err(general_err!("error reading PageLocation::first_row_index"));
+ }
+ let first_row_index = prot.read_i64()?;
+
+ // This loop slows things down a bit, but it's an acceptible price to allow
+ // forwards compatibility. We could instead assert the next field is Stop.
Review Comment:
likewise, if we are really focused on speed, we could do a fast path without a loop, and then add a fallback that handled it correctly
It would of course add complexity
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org