[GitHub] [arrow-rs] alamb commented on a diff in pull request #4151: Cleanup reading page index (#4149) (#4090)

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #4151:
URL: https://github.com/apache/arrow-rs/pull/4151#discussion_r1180800432


##########
parquet/src/file/metadata.rs:
##########
@@ -565,6 +566,13 @@ impl ColumnChunkMetaData {
         self.column_index_length
     }
 
+    /// Returns the range for the offset index if any
+    pub(crate) fn column_index_range(&self) -> Option<Range<usize>> {

Review Comment:
   Maybe we should make these functions pubic and deprecate the `offset_index_offset()` and `offset_index_length()` `column_index_offset()` and `column_index_length()` functions 🤔 `



##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -240,78 +243,53 @@ impl<T: AsyncFileReader + Send + 'static> ArrowReaderBuilder<AsyncReader<T>> {
             && metadata.column_index().is_none()
             && metadata.offset_index().is_none()
         {
-            let mut fetch_ranges = vec![];
-            let mut index_lengths: Vec<Vec<usize>> = vec![];
-
-            for rg in metadata.row_groups() {
-                let (loc_offset, loc_length) =
-                    index_reader::get_location_offset_and_total_length(rg.columns())?;
-
-                let (idx_offset, idx_lengths) =
-                    index_reader::get_index_offset_and_lengths(rg.columns())?;
-                let idx_length = idx_lengths.iter().sum::<usize>();
-
-                // If index data is missing, return without any indexes
-                if loc_length == 0 || idx_length == 0 {
-                    return Self::new_builder(AsyncReader(input), metadata, options);
-                }
-
-                fetch_ranges.push(loc_offset as usize..loc_offset as usize + loc_length);
-                fetch_ranges.push(idx_offset as usize..idx_offset as usize + idx_length);
-                index_lengths.push(idx_lengths);
-            }
-
-            let mut chunks = input.get_byte_ranges(fetch_ranges).await?.into_iter();
-            let mut index_lengths = index_lengths.into_iter();
-
-            let mut row_groups = metadata.row_groups().to_vec();
-
-            let mut columns_indexes = vec![];
-            let mut offset_indexes = vec![];
-
-            for rg in row_groups.iter_mut() {
-                let columns = rg.columns();
-
-                let location_data = chunks.next().unwrap();
-                let mut cursor = Cursor::new(location_data);
-                let mut offset_index = vec![];
-
-                for _ in 0..columns.len() {
-                    let mut prot = TCompactInputProtocol::new(&mut cursor);
-                    let offset = OffsetIndex::read_from_in_protocol(&mut prot)?;
-                    offset_index.push(offset.page_locations);
+            let fetch = metadata.row_groups().iter().flat_map(|r| r.columns()).fold(
+                None,
+                |a, c| {
+                    let a = acc_range(a, c.column_index_range());
+                    acc_range(a, c.offset_index_range())
+                },
+            );
+
+            if let Some(fetch) = fetch {
+                let bytes = input.get_bytes(fetch.clone()).await?;
+                let bytes = |r: Range<usize>| {

Review Comment:
   I was confused at first as the `bytes` closure shadowed the `bytes` actual data
   
   Maybe calling the closure something like `get_bytes` or `bytes_from_range` would be more readable 🤔 
   
   This same pattern appears several more times below in this PR



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