You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mapleFU (via GitHub)" <gi...@apache.org> on 2025/09/01 14:07:31 UTC
[PR] Parquet: Avoid page-size overflows i32 [arrow-rs]
mapleFU opened a new pull request, #8264:
URL: https://github.com/apache/arrow-rs/pull/8264
# Which issue does this PR close?
- Closes #8263
# Rationale for this change
See https://github.com/apache/arrow-rs/issues/8263
# What changes are included in this PR?
Avoid page overflows
# Are these changes tested?
* [ ] Will add
# 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Avoid page-size overflows i32 [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #8264:
URL: https://github.com/apache/arrow-rs/pull/8264#discussion_r2325346311
##########
parquet/src/file/writer.rs:
##########
@@ -966,7 +966,19 @@ impl<W: Write + Send> PageWriter for SerializedPageWriter<'_, W> {
let mut spec = PageWriteSpec::new();
spec.page_type = page_type;
spec.uncompressed_size = page.uncompressed_size() + header_size;
+ if spec.uncompressed_size > i32::MAX as usize {
Review Comment:
Or I can checking it when consuming, like in `to_thrift_header`
```rust
pub(crate) fn to_thrift_header(&self) -> PageHeader {
let uncompressed_size = self.uncompressed_size();
let compressed_size = self.compressed_size();
let num_values = self.num_values();
let encoding = self.encoding();
let page_type = self.page_type();
let mut page_header = PageHeader {
type_: page_type.into(),
uncompressed_page_size: uncompressed_size as i32,
compressed_page_size: compressed_size as i32,
// TODO: Add support for crc checksum
crc: None,
data_page_header: None,
index_page_header: None,
dictionary_page_header: None,
data_page_header_v2: 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Avoid page-size overflows i32 [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #8264:
URL: https://github.com/apache/arrow-rs/pull/8264#discussion_r2325375939
##########
parquet/src/column/page.rs:
##########
@@ -207,7 +207,6 @@ impl CompressedPage {
type_: page_type.into(),
uncompressed_page_size: uncompressed_size as i32,
compressed_page_size: compressed_size as i32,
- // TODO: Add support for crc checksum
Review Comment:
Removed
--
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] Parquet: Avoid page-size overflows i32 [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #8264:
URL: https://github.com/apache/arrow-rs/pull/8264#discussion_r2323926317
##########
parquet/src/file/writer.rs:
##########
@@ -966,7 +966,19 @@ impl<W: Write + Send> PageWriter for SerializedPageWriter<'_, W> {
let mut spec = PageWriteSpec::new();
spec.page_type = page_type;
spec.uncompressed_size = page.uncompressed_size() + header_size;
+ if spec.uncompressed_size > i32::MAX as usize {
Review Comment:
This is a "final around" before writing. Before `uncompressed_size` is using usize. I thinked about checking it when building `spec` but I don't know would it be too heavy ( or forcing )
--
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] Parquet: Avoid page-size overflows i32 [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #8264:
URL: https://github.com/apache/arrow-rs/pull/8264#issuecomment-3272911220
Actually I just meets a file which is corrupt by this is issue, cry :-(
```
{
"compression": "zstd",
"encoding": "delta_length_byte_array",
"page_type": "data_page_v2",
"offset": 5662247374,
"compressed_bytes": 581761248,
"uncompressed_bytes": -1963337762,
"header_bytes": 34,
"num_values": 10000
},
```
--
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] Parquet: Avoid page-size overflows i32 [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #8264:
URL: https://github.com/apache/arrow-rs/pull/8264#issuecomment-3271264156
I've change the logic to to_thrift_page ( which is just `pub(crate)` and commonly used) and add tests. Would you mind take a look again?
--
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] Parquet: Avoid page-size overflows i32 [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #8264:
URL: https://github.com/apache/arrow-rs/pull/8264#discussion_r2325341569
##########
parquet/src/file/writer.rs:
##########
@@ -966,7 +966,19 @@ impl<W: Write + Send> PageWriter for SerializedPageWriter<'_, W> {
let mut spec = PageWriteSpec::new();
spec.page_type = page_type;
spec.uncompressed_size = page.uncompressed_size() + header_size;
+ if spec.uncompressed_size > i32::MAX as usize {
Review Comment:
Now I have time for this part of the code. `pub struct PageWriteSpec` is widely used and it already uses usize. Should I just add setters like:`fn set_sizes(compressed, uncompressed) -> Result<()>`, or set the uncompressed_size to private and add visitors?
--
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] Parquet: Avoid page-size overflows i32 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8264:
URL: https://github.com/apache/arrow-rs/pull/8264
--
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] Parquet: Avoid page-size overflows i32 [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #8264:
URL: https://github.com/apache/arrow-rs/pull/8264#discussion_r2323926552
##########
parquet/src/file/writer.rs:
##########
@@ -966,7 +966,19 @@ impl<W: Write + Send> PageWriter for SerializedPageWriter<'_, W> {
let mut spec = PageWriteSpec::new();
spec.page_type = page_type;
spec.uncompressed_size = page.uncompressed_size() + header_size;
+ if spec.uncompressed_size > i32::MAX as usize {
Review Comment:
some setters is ok to me
--
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] Parquet: Avoid page-size overflows i32 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8264:
URL: https://github.com/apache/arrow-rs/pull/8264#discussion_r2322656939
##########
parquet/src/column/page.rs:
##########
@@ -207,7 +207,6 @@ impl CompressedPage {
type_: page_type.into(),
uncompressed_page_size: uncompressed_size as i32,
compressed_page_size: compressed_size as i32,
- // TODO: Add support for crc checksum
Review Comment:
This change is unrelated. AFAICT CRCs still aren't implemented on write.
##########
parquet/src/file/writer.rs:
##########
@@ -966,7 +966,19 @@ impl<W: Write + Send> PageWriter for SerializedPageWriter<'_, W> {
let mut spec = PageWriteSpec::new();
spec.page_type = page_type;
spec.uncompressed_size = page.uncompressed_size() + header_size;
+ if spec.uncompressed_size > i32::MAX as usize {
Review Comment:
I think we'll need this check everywhere `PageWriter` is implemented (namely also in `arrow/arrow_writer/mod.rs`). Perhaps it would be better to add some validation to the `PageWriteSpec` struct itself. Maybe add some setters?
--
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