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 2024/03/08 18:26:31 UTC
[PR] Add size statistics introduced in PARQUET-2261 [arrow-rs]
etseidl opened a new pull request, #5486:
URL: https://github.com/apache/arrow-rs/pull/5486
# Which issue does this PR close?
<!--
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. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #5022
# Rationale for this change
Implements new page and column chunk size statistics introduced in [PARQUET-2261](https://github.com/apache/parquet-format/pull/197)
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
Adds the necessary structures from the updated `parquet.thrift`, and adds the code necessary to populate them.
# Are there any user-facing changes?
No
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#discussion_r1676222642
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -538,6 +553,9 @@ pub struct ColumnChunkMetaData {
offset_index_length: Option<i32>,
column_index_offset: Option<i64>,
column_index_length: Option<i32>,
+ unencoded_byte_array_data_bytes: Option<i64>,
Review Comment:
done
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#issuecomment-2223340178
Thanks for the detailed and actionable review @alamb! I really appreciate you taking the time to review so thoroughly.
> If you are willing, I would like to suggest we break this code down into a few PRs:
Certainly. I can submit the format changes first, but I'll keep this open for the time being while I address your comments. Should I open new issues for each PR, or is it sufficient to reference #5022 for all three?
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#discussion_r1674370729
##########
parquet/src/file/page_index/offset_index.rs:
##########
@@ -0,0 +1,50 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! [`ParquetOffsetIndex`] structure holding decoded [`OffsetIndex`] information
+
+use crate::errors::ParquetError;
+use crate::format::{OffsetIndex, PageLocation};
+
+/// [`OffsetIndex`] information for a column chunk. Contains offsets and sizes for each page
+/// in the chunk. Optionally stores fully decoded page sizes for BYTE_ARRAY columns.
+#[derive(Debug, Clone, PartialEq)]
+pub struct ParquetOffsetIndex {
Review Comment:
I wasn't sure about removing the typedef since that would be a breaking change, no? (This structure isn't yet user visible IIUC). I can feel September coming on fast, so if you're ok with breaking changes at this point, I could take a stab at revamping all of the page index read support as one of the break-out PRs.
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#discussion_r1666090913
##########
parquet/src/column/writer/encoder.rs:
##########
@@ -150,6 +152,11 @@ impl<T: DataType> ColumnValueEncoderImpl<T> {
update_min(&self.descr, &min, &mut self.min_value);
update_max(&self.descr, &max, &mut self.max_value);
}
+
+ if let Some(var_bytes) = T::T::variable_length_bytes(slice) {
+ self.variable_length_bytes =
+ Some(var_bytes + self.variable_length_bytes.unwrap_or(0));
Review Comment:
Is this better as:
```suggestion
*self.variable_length_bytes.get_or_insert(0) += var_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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#discussion_r1673919034
##########
parquet/src/arrow/arrow_writer/byte_array.rs:
##########
@@ -269,12 +274,16 @@ impl FallbackEncoder {
}
};
+ let var_bytes = Some(self.variable_length_bytes);
Review Comment:
```suggestion
let variable_length_bytes = Some(self.variable_length_bytes);
```
Might be more consistent with the rest of the code. The same comment applies to several other uses below
##########
parquet/src/column/writer/mod.rs:
##########
@@ -275,6 +293,8 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E> {
num_buffered_values: 0,
num_buffered_rows: 0,
num_page_nulls: 0,
+ repetition_level_histogram: new_histogram_vec(max_rep_level),
Review Comment:
Reading through the code in this PR, I think it is hard for me to convince myself that the fields in PageMetrics are set / updated appropriately. Part of this is because the modifications are happening inline rather than, for example, some encapsulated methods of `PageMetrics`
I realize the this PR follows the existing pattern of modifing the fields of PageMetrics directly, but I think the new code is sufficiently complex that it is worth more encapsulation
For example, I would find it easier to reason about this code with something like
```rust
let mut page_metrics = PageMetrics::new();
// Collect histograms if there is more than a single level and if
// page or chunk statistics are being collected
if statistics_enabled == EnabledStatistics::None || max_level == 0 {
page_metrics = page_metrics
.with_repetition_level_histograms(max_rep_level)
.with_definition_level_histograms(max_def_level)
}
...
Self {
descr,
props,
...
page_metrics,
...
}
```
Likewise then below rather than inlining the update of ` repetition_level_histogram`
```rust
if let Some(ref mut def_hist) = self.page_metrics.definition_level_histogram {
// Count values and update histogram
for &level in levels {
process_def_level(level);
def_hist[level as usize] += 1;
}
} else {
// Count values
for &level in levels {
process_def_level(level);
}
}
```
It could be encapsulated into something like
```rust
self.page_metrics.update_definition_level_histogram(levels);
```
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -690,6 +708,26 @@ impl ColumnChunkMetaData {
Some(offset..(offset + length))
}
+ /// Returns the number of bytes of variable length data after decoding.
+ /// Only set for BYTE_ARRAY columns.
Review Comment:
I think it would be good to mention here that only some writers set this field
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -538,6 +553,9 @@ pub struct ColumnChunkMetaData {
offset_index_length: Option<i32>,
column_index_offset: Option<i64>,
column_index_length: Option<i32>,
+ unencoded_byte_array_data_bytes: Option<i64>,
Review Comment:
can we please add these fields to the HeapSize tracking: https://github.com/apache/arrow-rs/blob/e61fb621d1723b768689092d00cbce38ff8dc813/parquet/src/file/metadata/memory.rs#L95-L94 (which I think was added after you original wrote this PR)?
##########
parquet/src/column/writer/mod.rs:
##########
@@ -529,12 +552,26 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E> {
})?;
let mut values_to_write = 0;
- for &level in levels {
+
+ let mut process_def_level = |level| {
if level == self.descr.max_def_level() {
values_to_write += 1;
} else {
// We must always compute this as it is used to populate v2 pages
- self.page_metrics.num_page_nulls += 1
+ self.page_metrics.num_page_nulls += 1;
+ }
+ };
+
+ if let Some(ref mut def_hist) = self.page_metrics.definition_level_histogram {
Review Comment:
the same comment applies here about encapsulating the updates of page metrics -- I think it would reduce the repetition sigificantly
##########
parquet/src/column/writer/mod.rs:
##########
@@ -887,7 +989,11 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E> {
// Reset state.
self.rep_levels_sink.clear();
self.def_levels_sink.clear();
- self.page_metrics = PageMetrics::default();
+
+ // don't clobber histogram vectors
+ self.page_metrics.num_buffered_values = 0;
Review Comment:
I think the previous formulation was easier to reason about (reset page metrics)
##########
parquet/src/column/writer/mod.rs:
##########
@@ -782,8 +843,49 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E> {
_ => None,
};
+ if let Some(var_bytes) = values_data.variable_length_bytes {
+ *self.column_metrics.variable_length_bytes.get_or_insert(0) += var_bytes;
+ }
+
// update column and offset index
- self.update_column_offset_index(page_statistics.as_ref());
+ self.update_column_offset_index(
+ page_statistics.as_ref(),
+ values_data.variable_length_bytes,
+ );
+
+ // collect page histograms into chunk histograms and zero out page histograms
+ // TODO(ets): This could instead just add the vectors, and then allow page_metrics to be reset
+ // below. Would then need to recreate the histogram vectors, so `new_histogram_vec` above
+ // would need to become a function.
+ if let Some(ref mut page_hist) = self.page_metrics.repetition_level_histogram {
Review Comment:
If you followed the suggestion above to move more of this logic into `PageMetrics` maybe thie code could look like
```rust
self.column_metrics.update_from_page(&self.page_metrics);
// reset metrics for a new page (zeros out buffered rows, resets histogram counts to 0)
self.page_metrics.new_page()
...
```
##########
parquet/src/file/page_index/index.rs:
##########
@@ -41,6 +41,10 @@ pub struct PageIndex<T> {
pub max: Option<T>,
/// Null values in the page
pub null_count: Option<i64>,
+ /// Repetition level histogram for the page
Review Comment:
Could we also note here that each element is the count of the repetition / definition level?
##########
parquet/src/format.rs:
##########
@@ -634,6 +634,143 @@ impl From<&BoundaryOrder> for i32 {
}
}
+//
Review Comment:
How was this re-generated? Do we have to update a pin to parquet-format anywhere?
##########
parquet/src/file/writer.rs:
##########
@@ -1828,4 +1836,235 @@ mod tests {
let b_idx = &column_index[0][1];
assert!(matches!(b_idx, Index::NONE), "{b_idx:?}");
}
+
+ #[test]
+ fn test_byte_array_size_statistics() {
+ let message_type = "
+ message test_schema {
+ OPTIONAL BYTE_ARRAY a (UTF8);
+ }
+ ";
+ let schema = Arc::new(parse_message_type(message_type).unwrap());
+ let data = ByteArrayType::gen_vec(32, 7);
+ let def_levels = [1, 1, 1, 1, 0, 1, 0, 1, 0, 1];
+ let unenc_size: i64 = data.iter().map(|x| x.len() as i64).sum();
+ let file: File = tempfile::tempfile().unwrap();
+ let props = Arc::new(
+ WriterProperties::builder()
+ .set_statistics_enabled(EnabledStatistics::Page)
+ .build(),
+ );
+
+ let mut writer = SerializedFileWriter::new(&file, schema, props).unwrap();
+ let mut row_group_writer = writer.next_row_group().unwrap();
+
+ let mut col_writer = row_group_writer.next_column().unwrap().unwrap();
+ col_writer
+ .typed::<ByteArrayType>()
+ .write_batch(&data, Some(&def_levels), None)
+ .unwrap();
+ col_writer.close().unwrap();
+ row_group_writer.close().unwrap();
+ let file_metadata = writer.close().unwrap();
+
+ assert_eq!(file_metadata.row_groups.len(), 1);
+ assert_eq!(file_metadata.row_groups[0].columns.len(), 1);
+ assert!(file_metadata.row_groups[0].columns[0].meta_data.is_some());
+
+ let check_def_hist = |def_hist: &Vec<i64>| {
+ assert_eq!(def_hist.len(), 2);
+ assert_eq!(def_hist[0], 3);
+ assert_eq!(def_hist[1], 7);
+ };
+
+ if let Some(ref meta_data) = file_metadata.row_groups[0].columns[0].meta_data {
Review Comment:
I think the test should asset if this field is actually set -- there shouldn't be any non determinism
Like for example
```rust
let Some(meta_data) = file_metadata.row_groups[0].columns[0].meta_data.as_ref().unwrap())
```
##########
parquet/src/file/page_index/index_reader.rs:
##########
@@ -109,6 +110,48 @@ pub fn read_pages_locations<R: ChunkReader>(
.collect()
}
+/// Reads per-column [`ParquetOffsetIndex`] for all columns of a row group by
+/// decoding [`OffsetIndex`] .
+///
+/// Returns a vector of `index[column_number]`.
+///
+/// Returns an empty vector if this row group does not contain an
+/// [`OffsetIndex`].
+///
+/// See [Offset Index Documentation] for more details.
Review Comment:
I think this should be `Page Index Documentation`
I find the naming of the structs in parquet-rs quite confusing here (the parquet "PageIndex" is comprised of OffsetIndex and page index)
##########
parquet/src/file/page_index/offset_index.rs:
##########
@@ -0,0 +1,50 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! [`ParquetOffsetIndex`] structure holding decoded [`OffsetIndex`] information
+
+use crate::errors::ParquetError;
+use crate::format::{OffsetIndex, PageLocation};
+
+/// [`OffsetIndex`] information for a column chunk. Contains offsets and sizes for each page
+/// in the chunk. Optionally stores fully decoded page sizes for BYTE_ARRAY columns.
+#[derive(Debug, Clone, PartialEq)]
+pub struct ParquetOffsetIndex {
Review Comment:
This is an excellent change (to make ParquetOffsetIndex an *actual* structure rather than a typedef).
I am surprised that the other implementation of https://github.com/apache/arrow-rs/blob/a008e9ed6d8cb6667af3ee40d4b3880de2c5c9ee/parquet/src/file/metadata/mod.rs#L72-L71 doesn't seem to be removed in this PR 🤔
I have actually wanted to give `ParquetColumnIndex` the same treatment as well
##########
parquet/src/file/writer.rs:
##########
@@ -1828,4 +1836,235 @@ mod tests {
let b_idx = &column_index[0][1];
assert!(matches!(b_idx, Index::NONE), "{b_idx:?}");
}
+
+ #[test]
+ fn test_byte_array_size_statistics() {
+ let message_type = "
+ message test_schema {
+ OPTIONAL BYTE_ARRAY a (UTF8);
+ }
+ ";
+ let schema = Arc::new(parse_message_type(message_type).unwrap());
+ let data = ByteArrayType::gen_vec(32, 7);
+ let def_levels = [1, 1, 1, 1, 0, 1, 0, 1, 0, 1];
+ let unenc_size: i64 = data.iter().map(|x| x.len() as i64).sum();
+ let file: File = tempfile::tempfile().unwrap();
+ let props = Arc::new(
+ WriterProperties::builder()
+ .set_statistics_enabled(EnabledStatistics::Page)
+ .build(),
+ );
+
+ let mut writer = SerializedFileWriter::new(&file, schema, props).unwrap();
+ let mut row_group_writer = writer.next_row_group().unwrap();
+
+ let mut col_writer = row_group_writer.next_column().unwrap().unwrap();
+ col_writer
+ .typed::<ByteArrayType>()
+ .write_batch(&data, Some(&def_levels), None)
+ .unwrap();
+ col_writer.close().unwrap();
+ row_group_writer.close().unwrap();
+ let file_metadata = writer.close().unwrap();
+
+ assert_eq!(file_metadata.row_groups.len(), 1);
+ assert_eq!(file_metadata.row_groups[0].columns.len(), 1);
+ assert!(file_metadata.row_groups[0].columns[0].meta_data.is_some());
+
+ let check_def_hist = |def_hist: &Vec<i64>| {
+ assert_eq!(def_hist.len(), 2);
+ assert_eq!(def_hist[0], 3);
+ assert_eq!(def_hist[1], 7);
+ };
+
+ if let Some(ref meta_data) = file_metadata.row_groups[0].columns[0].meta_data {
Review Comment:
same thing below
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#issuecomment-2204567034
@alamb I have the offset index done now well enough to test the metadata (might still need doc updates). I'm not entirely happy with it since the offset index is parsed twice now. I'm fine with either proceeding with this and then submitting a follow-on PR to add a proper offset index struct to hold the page locations and unencoded array sizes, or just doing both in this PR and waiting until 53.0.0.
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#issuecomment-2226845164
> Certainly. I can submit the format changes first, but I'll keep this open for the time being while I address your comments. Should I open new issues for each PR, or is it sufficient to reference #5022 for all three?
I think using one ticket is just fine. No need to make new tickets
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#issuecomment-2253341666
🎉
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#discussion_r1674344471
##########
parquet/src/format.rs:
##########
@@ -634,6 +634,143 @@ impl From<&BoundaryOrder> for i32 {
}
}
+//
Review Comment:
I manually downloaded https://github.com/apache/parquet-format/commit/40699d05bd24181de6b1457babbee2c16dce3803 and ran thrift. Then I discovered the `regen.sh` and performed the steps therein. Yes, the `REVISION` in `regen.sh` should be updated. I'll make that part of a new PR for the format changes.
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#discussion_r1674358884
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -538,6 +553,9 @@ pub struct ColumnChunkMetaData {
offset_index_length: Option<i32>,
column_index_offset: Option<i64>,
column_index_length: Option<i32>,
+ unencoded_byte_array_data_bytes: Option<i64>,
Review Comment:
Sure. I should have thought to do that when I noticed the heap size tests failing after a merge.
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#issuecomment-2226008759
@alamb I think I've addressed most of your concerns. Please take a look when you have a spare moment. I'm going to set this PR back to draft and begin on splitting into at least 2 more PRs (I've already submitted #6045 for the thrift component).
--
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] Add size statistics introduced in PARQUET-2261 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#issuecomment-1991439073
I triggered the CI checks and will try and get to review this over the next few days if no one beats me to 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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl closed pull request #5486: Add size statistics to `ParquetMetaData` introduced in PARQUET-2261
URL: https://github.com/apache/arrow-rs/pull/5486
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#issuecomment-2253240506
Superseded by #6105 and others
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#issuecomment-2226957085
> Update: what do you think about doing incremental PRs to a feature branch? #6050
Sounds good to me. We can all use #6050 to discuss deconfliction in the event of conflicts. And we can afford to be a bit more free wheeling too 😄. For instance, I have a branch for changing the `offset_index` in `ParquetMetaData` to a `Vec<Vec<ParquetOffsetIndex>>`. I almost submitted that PR, but held back because the ripple of changes throughout the parquet code was a bit more than I expected. With the feature branch, I'll feel more comfortable going at a slower rate (add the variable length stats gathering, then change the API, then add histograms, then (perhaps) change the column index API to be consistent with the offset index.
Thank you for helping to coordinate all this ❤️
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#issuecomment-2226873202
Update: what do you think about doing incremental PRs to a feature branch? https://github.com/apache/arrow-rs/issues/6050
--
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] Add size statistics introduced in PARQUET-2261 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#issuecomment-1986201497
This is my first foray into Rust programming, so I'm not sure everything is done as idiomatically as possible. Submitting this now to get early feedback on my approach. I'm also wondering how much testing to add, and whether this should have a configuration parameter to turn generating the statistics off.
--
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] Add size statistics to `ParquetMetaData` introduced in PARQUET-2261 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5486:
URL: https://github.com/apache/arrow-rs/pull/5486#issuecomment-2212428354
Thanks @etseidl -- I will try and find time this week to review this PR in more detail
--
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