You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2024/08/28 17:25:37 UTC
[PR] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
adriangb opened a new pull request, #6319:
URL: https://github.com/apache/arrow-rs/pull/6319
Fixes https://github.com/apache/arrow-rs/issues/6310
--
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] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319#discussion_r1735078046
##########
parquet/src/file/metadata/writer.rs:
##########
@@ -450,19 +451,57 @@ mod tests {
true,
)]));
- let a: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), None, Some(2)]));
+ // build row groups / pages that exercise different combinations of nulls and values
Review Comment:
I like these test additions, particularly that they build off of the tests added in #6197. They also seem much more thorough than what I proposed 😄 .
##########
parquet/src/file/page_index/index.rs:
##########
@@ -227,19 +227,16 @@ impl<T: ParquetValueType> NativeIndex<T> {
}
pub(crate) fn to_thrift(&self) -> ColumnIndex {
- let min_values = self
- .indexes
- .iter()
- .map(|x| x.min_bytes().map(|x| x.to_vec()))
- .collect::<Option<Vec<_>>>()
- .unwrap_or_else(|| vec![vec![]; self.indexes.len()]);
-
- let max_values = self
- .indexes
- .iter()
- .map(|x| x.max_bytes().map(|x| x.to_vec()))
- .collect::<Option<Vec<_>>>()
- .unwrap_or_else(|| vec![vec![]; self.indexes.len()]);
+ let mut min_values = vec![vec![]; self.indexes.len()];
+ let mut max_values = vec![vec![]; self.indexes.len()];
+ for (i, index) in self.indexes.iter().enumerate() {
+ if let Some(min) = index.min_bytes() {
+ min_values[i].extend_from_slice(min);
+ }
+ if let Some(max) = index.max_bytes() {
+ max_values[i].extend_from_slice(max);
+ }
+ }
Review Comment:
I know I'm responsible for this, but now I wonder if simply changing the above to something like:
```rust
let min_values = self
.indexes
.iter()
.map(|x| x.min_bytes().unwrap_or(&[]).to_vec())
.collect::<Vec<_>>();
```
would be more rusty?
--
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] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319#discussion_r1735071644
##########
parquet/src/column/writer/mod.rs:
##########
@@ -756,8 +756,8 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E> {
if null_page && self.column_index_builder.valid() {
self.column_index_builder.append(
null_page,
- vec![0; 1],
- vec![0; 1],
+ vec![],
+ vec![],
Review Comment:
reverted it and pushed a single commit that also credits you 😄
--
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] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319#discussion_r1735059501
##########
parquet/src/file/metadata/writer.rs:
##########
@@ -450,19 +451,47 @@ mod tests {
true,
)]));
- let a: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), None, Some(2)]));
+ // build row groups / pages that exercise different combinations of nulls and values
+ // note that below we set the row group and page sizes to 4 and 2 respectively
+ // so that these "groupings" make sense
+ let a: ArrayRef = Arc::new(Int32Array::from(vec![
+ // a row group that has all values
+ Some(i32::MIN), Some(-1),
+ Some(1), Some(i32::MAX),
+ // a row group with a page of all nulls and a page of all values
+ None, None,
+ Some(2), Some(3),
+ // a row group that has all null pages
+ None, None,
+ None, None,
+ // a row group having 1 page with all values and 1 page with some nulls
+ Some(4), Some(5),
+ None, Some(6),
+ // a row group having 1 page with all nulls and 1 page with some nulls
+ None, None,
+ Some(7), None,
+ // a row group having all pages with some nulls
+ None, Some(8),
+ Some(9), None,
+ ]));
let batch = RecordBatch::try_from_iter(vec![("a", a)]).unwrap();
- let writer_props = match write_page_index {
+ let writer_props_builder = match write_page_index {
true => WriterProperties::builder()
- .set_statistics_enabled(EnabledStatistics::Page)
- .build(),
+ .set_statistics_enabled(EnabledStatistics::Page),
false => WriterProperties::builder()
- .set_statistics_enabled(EnabledStatistics::Chunk)
- .build(),
+ .set_statistics_enabled(EnabledStatistics::Chunk),
};
+ // tune the size or pages to the data above
+ // to make sure we exercise code paths where all items in a page are null, etc.
+ let writer_props = writer_props_builder
+ .set_max_row_group_size(4)
+ .set_data_page_row_count_limit(2)
+ .set_write_batch_size(2)
Review Comment:
I recognize that this is forcing things a bit. I leaned towards this approach because (1) it felt like a small step from where we are now and (2) I'm not that familiar with the lower level APIs that would be required if we wanted to build a `ParquetMetadata` by hand and use that for tests instead of relying on file writing to build one for us. I think a reasonable piece of feedback to this PR would be that now is the time to refactor the tests to build the `ParquetMetadata` by hand since we now care about getting pages of a certain size, 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319#discussion_r1735065816
##########
parquet/src/file/metadata/writer.rs:
##########
@@ -610,6 +649,29 @@ mod tests {
decoded_metadata.num_row_groups()
);
+ // check that the mins and maxes are what we expect for each page
+ // also indirectly checking that the pages were written out as we expected them to be laid out
+ // (if they're not, or something gets refactored in the future that breaks that assumption,
+ // this test may have to drop down to a lower level and create metadata directly instead of relying on
+ // writing an entire file)
+ let column_indexes = metadata.metadata.column_index().unwrap();
+ assert_eq!(column_indexes.len(), 6);
+ // make sure each row group has 2 pages by checking the first column
+ // page counts for each column for each row group, should all be the same and there should be
+ // 12 pages in total across 6 row groups / 1 column
+ let mut page_counts = vec![];
+ for row_group in column_indexes {
+ for column in row_group {
+ match column {
+ Index::INT32(column_index) => {
+ page_counts.push(column_index.indexes.len());
+ }
+ _ => panic!("unexpected column index type"),
+ }
+ }
+ }
+ assert_eq!(page_counts, vec![2; 6]);
Review Comment:
Unless we refactor this test to create the metadata directly I think this is a good idea. Previously there was no assertion that we were creating more than 1 page or anything of the sort, since this is testing code that evidently diverges in code paths depending on how many pages there are, etc. it's good to at least have assertions that the data is laid out as we expect it to be.
--
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] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319#discussion_r1739709427
##########
parquet/src/file/metadata/writer.rs:
##########
@@ -554,8 +583,8 @@ mod tests {
assert_eq!(left.compressed_size(), right.compressed_size());
assert_eq!(left.data_page_offset(), right.data_page_offset());
assert_eq!(left.statistics(), right.statistics());
- assert_eq!(left.offset_index_length(), right.offset_index_length());
- assert_eq!(left.column_index_length(), right.column_index_length());
+ // assert_eq!(left.offset_index_length(), right.offset_index_length());
+ // assert_eq!(left.column_index_length(), right.column_index_length());
Review Comment:
re-enabled in https://github.com/apache/arrow-rs/pull/6319/commits/8eea1e5899b46d5236a4d9fe9e0f56f46f168b65
--
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] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319
--
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] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319#issuecomment-2316149331
I'm not a committer so I have no power here 😅. @alamb will hopefully take a look once his vacation is over.
--
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] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319#discussion_r1739708945
##########
parquet/src/file/metadata/writer.rs:
##########
@@ -450,19 +451,47 @@ mod tests {
true,
)]));
- let a: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), None, Some(2)]));
+ // build row groups / pages that exercise different combinations of nulls and values
+ // note that below we set the row group and page sizes to 4 and 2 respectively
+ // so that these "groupings" make sense
+ let a: ArrayRef = Arc::new(Int32Array::from(vec![
+ // a row group that has all values
+ Some(i32::MIN), Some(-1),
+ Some(1), Some(i32::MAX),
+ // a row group with a page of all nulls and a page of all values
+ None, None,
+ Some(2), Some(3),
+ // a row group that has all null pages
+ None, None,
+ None, None,
+ // a row group having 1 page with all values and 1 page with some nulls
+ Some(4), Some(5),
+ None, Some(6),
+ // a row group having 1 page with all nulls and 1 page with some nulls
+ None, None,
+ Some(7), None,
+ // a row group having all pages with some nulls
+ None, Some(8),
+ Some(9), None,
+ ]));
let batch = RecordBatch::try_from_iter(vec![("a", a)]).unwrap();
- let writer_props = match write_page_index {
+ let writer_props_builder = match write_page_index {
true => WriterProperties::builder()
- .set_statistics_enabled(EnabledStatistics::Page)
- .build(),
+ .set_statistics_enabled(EnabledStatistics::Page),
false => WriterProperties::builder()
- .set_statistics_enabled(EnabledStatistics::Chunk)
- .build(),
+ .set_statistics_enabled(EnabledStatistics::Chunk),
};
+ // tune the size or pages to the data above
+ // to make sure we exercise code paths where all items in a page are null, etc.
+ let writer_props = writer_props_builder
+ .set_max_row_group_size(4)
+ .set_data_page_row_count_limit(2)
+ .set_write_batch_size(2)
Review Comment:
I thik this is reasonable
--
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] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319#discussion_r1735056465
##########
parquet/src/file/metadata/writer.rs:
##########
@@ -554,8 +583,8 @@ mod tests {
assert_eq!(left.compressed_size(), right.compressed_size());
assert_eq!(left.data_page_offset(), right.data_page_offset());
assert_eq!(left.statistics(), right.statistics());
- assert_eq!(left.offset_index_length(), right.offset_index_length());
- assert_eq!(left.column_index_length(), right.column_index_length());
+ // assert_eq!(left.offset_index_length(), right.offset_index_length());
+ // assert_eq!(left.column_index_length(), right.column_index_length());
Review Comment:
With just the changes to this file (so the new test data) this fails on `main`, it's because of https://github.com/apache/arrow-rs/pull/6316/files so I commented it out for now and if:
1. #6316 gets merged first I can rebase and re-enable the code
2. If this gets merged first then #6316 can uncomment and re-enable this code
--
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] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319#discussion_r1735056465
##########
parquet/src/file/metadata/writer.rs:
##########
@@ -554,8 +583,8 @@ mod tests {
assert_eq!(left.compressed_size(), right.compressed_size());
assert_eq!(left.data_page_offset(), right.data_page_offset());
assert_eq!(left.statistics(), right.statistics());
- assert_eq!(left.offset_index_length(), right.offset_index_length());
- assert_eq!(left.column_index_length(), right.column_index_length());
+ // assert_eq!(left.offset_index_length(), right.offset_index_length());
+ // assert_eq!(left.column_index_length(), right.column_index_length());
Review Comment:
With just the changes to this file (so the new test data) this fails on `master` because of https://github.com/apache/arrow-rs/pull/6316/, so I commented it out for now and if:
1. #6316 gets merged first I can rebase and re-enable the code
2. If this gets merged first then #6316 can uncomment and re-enable this code
--
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] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319#issuecomment-2322887215
🔧
--
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] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319#issuecomment-2316142209
Thank you @etseidl! Can we merge this or do we need to loop in some more folks?
--
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] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319#discussion_r1735067813
##########
parquet/src/column/writer/mod.rs:
##########
@@ -756,8 +756,8 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E> {
if null_page && self.column_index_builder.valid() {
self.column_index_builder.append(
null_page,
- vec![0; 1],
- vec![0; 1],
+ vec![],
+ vec![],
Review Comment:
Oops, that's reading ahead 🤣
--
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] Fix writing of invalid Parquet ColumnIndex when row group contains null pages [arrow-rs]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #6319:
URL: https://github.com/apache/arrow-rs/pull/6319#discussion_r1735105320
##########
parquet/src/file/page_index/index.rs:
##########
@@ -227,19 +227,16 @@ impl<T: ParquetValueType> NativeIndex<T> {
}
pub(crate) fn to_thrift(&self) -> ColumnIndex {
- let min_values = self
- .indexes
- .iter()
- .map(|x| x.min_bytes().map(|x| x.to_vec()))
- .collect::<Option<Vec<_>>>()
- .unwrap_or_else(|| vec![vec![]; self.indexes.len()]);
-
- let max_values = self
- .indexes
- .iter()
- .map(|x| x.max_bytes().map(|x| x.to_vec()))
- .collect::<Option<Vec<_>>>()
- .unwrap_or_else(|| vec![vec![]; self.indexes.len()]);
+ let mut min_values = vec![vec![]; self.indexes.len()];
+ let mut max_values = vec![vec![]; self.indexes.len()];
+ for (i, index) in self.indexes.iter().enumerate() {
+ if let Some(min) = index.min_bytes() {
+ min_values[i].extend_from_slice(min);
+ }
+ if let Some(max) = index.max_bytes() {
+ max_values[i].extend_from_slice(max);
+ }
+ }
Review Comment:
yup i think it would be, changed
--
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