StatisticsConverter::row_group_null_counts incorrect for missing column #10926

Activity

1. alamb

   added

   bugSomething isn't workingSomething isn't working

   on Jun 15, 2024
2. marvinlanhenke commented on Jun 16, 2024

   

   marvinlanhenke

   on Jun 16, 2024

   Contributor

   More actions

   take
3. github-actions

   assigned

   marvinlanhenke

   on Jun 16, 2024
4. marvinlanhenke commented on Jun 16, 2024

   

   marvinlanhenke

   on Jun 16, 2024

   Last edited by marvinlanhenke

   Contributor

   More actions

   @alamb
   I have two questions here:

   Should we also change the signature and return Result<Uint64Array>?
   This however, would require to downcast_ref the result from new_null_array and clone the result.

   Something like:

   let Some(parquet_index) = self.parquet_index else {
               return Ok(self
                   .make_null_array(&DataType::UInt64, metadatas)
                   .as_any()
                   .downcast_ref::<UInt64Array>()
                   .expect("failed to downcast array")
                   .clone());
           };

   If we change the signature we also have to make some changes downstream, e.g. in row_groups.rs fn null_counts

   Yet another question, what is the correct behaviour in data_page_row_counts if we cannot find a matching parquet_index, should we return a null_array (like we do already) or provide the row_group_row_count instead, since we cannot access the pages via parquet_index anyway? Is it even correct to not return a null-array in row_group_row_counts even if we can't match the column?

   EDIT: I guess this answers the second question:

   If there is not a column, the UInt64Array should be all nulls
5. alamb commented on Jun 16, 2024

   

   alamb

   on Jun 16, 2024

   ContributorAuthor

   More actions

   Should we also change the signature and return Result?

   Yes I think so

   Should we also change the signature and return Result?

   I actually think using new_null_array is wrong here. It returns the wrong type -- for example if we are getting row counts for a Utf8 column, using new_null_arrray will return a StringArray rather than a UInt64Array.

   I actually hit this issue when working on #10924

   And I had to change it to

           let Some(parquet_index) = self.parquet_index else {
               let num_row_groups = metadatas.into_iter().count();
               return Ok(Arc::new(UInt64Array::from_iter(
                   std::iter::repeat(None).take(num_row_groups),
               )));
           };

   To make a test pass

   👍React with 👍1Reacted by Marvin Lanhenke
6. marvinlanhenke commented on Jun 16, 2024

   

   marvinlanhenke

   on Jun 16, 2024

   Last edited by marvinlanhenke

   Contributor

   More actions

   I actually think using new_null_array is wrong here. It returns the wrong type

   I passed &DataType::UInt64 into new_null_array(...). However, your approach solves the issue with downcasting the result from new_null_array so it makes more sense here. Thank you.

   @alamb just to confirm my understanding (regarding my second question) row_group_row_counts should also return a null_array when we reference a non-existing column; thus regardless if row group or data page both row_counts behave the same way when encountering a missing col?

   👍React with 👍1Reacted by Andrew Lamb
7. alamb

   closed this as completedin #10946on Jun 17, 2024