Expose `SortingColumn` in parquet files by askoa · Pull Request #3103 · apache/arrow-rs

askoa · 2022-11-13T22:57:22Z

Which issue does this PR close?

Reading from file:

The function footer.rs#decode_metadata read sorting_column from file. However the function RowGroupMetaData::from_thrift was not reading the field into RowGroupMetaData. The function was modified to read the sorting_column into RowGroupMetaData

arrow-rs/parquet/src/file/footer.rs

Lines 74 to 82 in 430eb84

    
           let t_file_metadata: TFileMetaData = TFileMetaData::read_from_in_protocol(&mut prot) 
        
               .map_err(|e| ParquetError::General(format!("Could not parse metadata: {}", e)))?; 
        
           let schema = types::from_thrift(&t_file_metadata.schema)?; 
        
           let schema_descr = Arc::new(SchemaDescriptor::new(schema)); 
        
           let mut row_groups = Vec::new(); 
        
           for rg in t_file_metadata.row_groups { 
        
               row_groups.push(RowGroupMetaData::from_thrift(schema_descr.clone(), rg)?); 
        
           } 
        
           let column_orders = parse_column_orders(t_file_metadata.column_orders, &schema_descr);

Writing into file:

The function format.rs#RowGroup#write_to_out_protocol writes sorting_column to file. However the function metadata.rs#RowGroupMetaData#to_thrift was not writing the field to RowGroup. The function was modified to write sorting_column from RowGroupMetaData to RowGroup

arrow-rs/parquet/src/format.rs

Lines 4155 to 4162 in 430eb84

    
           if let Some(ref fld_var) = self.sorting_columns { 
        
             o_prot.write_field_begin(&TFieldIdentifier::new("sorting_columns", TType::List, 4))?; 
        
             o_prot.write_list_begin(&TListIdentifier::new(TType::Struct, fld_var.len() as i32))?; 
        
             for e in fld_var { 
        
               e.write_to_out_protocol(o_prot)?; 
        
             } 
        
             o_prot.write_list_end()?; 
        
             o_prot.write_field_end()?

tustvold

Could we get an end-to-end test of this, i.e. write a parquet file to a Vec<u8> then read it back and verify the sort column was round-tripped correctly

parquet/src/file/metadata.rs

tustvold · 2022-11-14T04:37:01Z

parquet/src/file/metadata.rs

Suggested change

value: Option<Vec<SortingColumnMetaData>>,

) -> Self {

self.sorting_columns = value;

value: Vec<SortingColumnMetaData>,

) -> Self {

self.sorting_columns = Some(value);

This is consistent with set_page_offset

I got a reverse review before. The reason given by the reviewer was that if we remove Option from signature then the function cannot be used to set None for this field. I am going to keep this as is.

parquet/src/file/metadata.rs

tustvold

Getting close 😄

parquet/src/file/metadata.rs

parquet/tests/parquet_round_trip.rs

parquet/src/file/metadata.rs

parquet/tests/parquet_round_trip.rs

parquet/src/file/metadata.rs

…ing-column

tustvold · 2022-11-15T19:34:43Z

Thank you 👍

ursabot · 2022-11-15T19:42:58Z

Benchmark runs are scheduled for baseline = 8bb2917 and contender = 371ec57. 371ec57 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the parquet Changes to the parquet crate label Nov 13, 2022

askoa marked this pull request as ready for review November 13, 2022 23:57

tustvold reviewed Nov 14, 2022

View reviewed changes

askoa marked this pull request as draft November 14, 2022 13:57

askoa added 7 commits November 14, 2022 20:12

Expose SortColumn from parquet file

15a2bc9

fix formatting issues

c5cfd1f

empty commit

2bf03df

fix PR comments

09d16e1

formatting fix

8b0ad0f

add parquet round trip test

d83f10e

fix clippy error

eecaafa

askoa marked this pull request as ready for review November 15, 2022 01:26

tustvold reviewed Nov 15, 2022

View reviewed changes

tustvold mentioned this pull request Nov 15, 2022

Cleanup parquet tests #3116

Merged

askoa added 2 commits November 15, 2022 06:56

Merge branch 'master' of https://github.com/apache/arrow-rs into sort…

d5ea969

…ing-column

update the test based on PR comment

7e89d52

tustvold approved these changes Nov 15, 2022

View reviewed changes

tustvold merged commit 371ec57 into apache:master Nov 15, 2022

askoa deleted the sorting-column branch December 1, 2022 15:10

zhuqi-lucas mentioned this pull request Dec 26, 2024

Find a way to communicate the ordering of a file back with the existing listing table implementation apache/datafusion#13891

Open

	let t_file_metadata: TFileMetaData = TFileMetaData::read_from_in_protocol(&mut prot)
	.map_err(\|e\| ParquetError::General(format!("Could not parse metadata: {}", e)))?;
	let schema = types::from_thrift(&t_file_metadata.schema)?;
	let schema_descr = Arc::new(SchemaDescriptor::new(schema));
	let mut row_groups = Vec::new();
	for rg in t_file_metadata.row_groups {
	row_groups.push(RowGroupMetaData::from_thrift(schema_descr.clone(), rg)?);
	}
	let column_orders = parse_column_orders(t_file_metadata.column_orders, &schema_descr);

	if let Some(ref fld_var) = self.sorting_columns {
	o_prot.write_field_begin(&TFieldIdentifier::new("sorting_columns", TType::List, 4))?;
	o_prot.write_list_begin(&TListIdentifier::new(TType::Struct, fld_var.len() as i32))?;
	for e in fld_var {
	e.write_to_out_protocol(o_prot)?;
	}
	o_prot.write_list_end()?;
	o_prot.write_field_end()?

-        value: Option<Vec<SortingColumnMetaData>>,
-    ) -> Self {
-        self.sorting_columns = value;
+        value: Vec<SortingColumnMetaData>,
+    ) -> Self {
+        self.sorting_columns = Some(value);

Conversation

askoa commented Nov 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Reading from file:

Writing into file:

Uh oh!

tustvold left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tustvold Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

askoa Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tustvold left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tustvold commented Nov 15, 2022

Uh oh!

ursabot commented Nov 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

askoa commented Nov 13, 2022 •

edited

Loading