Skip to content

Add column index writer for parquet#1935

Merged
tustvold merged 1 commit intoapache:masterfrom
liukun4515:parquet_column_index_write
Jun 30, 2022
Merged

Add column index writer for parquet#1935
tustvold merged 1 commit intoapache:masterfrom
liukun4515:parquet_column_index_write

Conversation

@liukun4515
Copy link
Contributor

@liukun4515 liukun4515 commented Jun 24, 2022

Which issue does this PR close?

part of #1777

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the parquet Changes to the parquet crate label Jun 24, 2022
@liukun4515 liukun4515 changed the title add column index writer for parquet [WIP] add column index writer for parquet Jun 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2022 •
edited
Loading

Codecov Report

Merging #1935 (2ec0dfa) into master (9059cbf) will increase coverage by 0.12%.
The diff coverage is 88.34%.

❗ Current head 2ec0dfa differs from pull request most recent head a5faeb6. Consider uploading reports for the commit a5faeb6 to get more accurate results

@@            Coverage Diff             @@
##           master    #1935      +/-   ##
==========================================
+ Coverage   83.41%   83.54%   +0.12%     
==========================================
  Files         214      221       +7     
  Lines       57004    57395     +391     
==========================================
+ Hits        47551    47949     +398     
+ Misses       9453     9446       -7
Impacted Files Coverage Δ
arrow/src/alloc/mod.rs 78.04% <ø> (ø)
arrow/src/array/array_dictionary.rs 91.53% <ø> (ø)
arrow/src/array/array_list.rs 96.16% <ø> (ø)
arrow/src/array/builder/decimal_builder.rs 76.81% <0.00%> (+7.62%) ⬆️
arrow/src/array/mod.rs 100.00% <ø> (ø)
arrow/src/buffer/immutable.rs 98.48% <ø> (ø)
arrow/src/bytes.rs 73.07% <ø> (ø)
arrow/src/datatypes/datatype.rs 65.42% <ø> (ø)
parquet/src/arrow/array_reader/mod.rs 88.23% <ø> (-2.54%) ⬇️
parquet/src/arrow/arrow_writer/mod.rs 97.53% <ø> (ø)
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9059cbf...a5faeb6. Read the comment docs.

@liukun4515 liukun4515 force-pushed the parquet_column_index_write branch from 04f164a to e705846 Compare June 24, 2022 12:46
@liukun4515 liukun4515 mentioned this pull request Jun 25, 2022
Closed
@liukun4515 liukun4515 force-pushed the parquet_column_index_write branch 5 times, most recently from 735f4ed to 43512f5 Compare June 28, 2022 06:01
liukun4515
liukun4515 commented Jun 28, 2022
View reviewed changes
liukun4515
liukun4515 commented Jun 28, 2022
View reviewed changes
@liukun4515 liukun4515 force-pushed the parquet_column_index_write branch 2 times, most recently from ea799a9 to 798af4f Compare June 28, 2022 07:20
@liukun4515 liukun4515 changed the title [WIP] add column index writer for parquet add column index writer for parquet Jun 28, 2022
@liukun4515 liukun4515 requested review from alamb, sunchao and tustvold June 28, 2022 07:21
@liukun4515 liukun4515 marked this pull request as ready for review June 28, 2022 07:21
@liukun4515 liukun4515 force-pushed the parquet_column_index_write branch from 798af4f to f76ff65 Compare June 28, 2022 07:52
@liukun4515
Copy link
Contributor Author

liukun4515 commented Jun 28, 2022

@Ted-Jiang PTAL

@liukun4515
Copy link
Contributor Author

liukun4515 commented Jun 28, 2022

Do we need to add an option to control this feature in the WriterProperties struct?

tustvold
tustvold reviewed Jun 28, 2022
View reviewed changes
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking very nice, I think it would be good to maintain a clearer separation between file-level metadata and the index metadata. Mixing the two not only leads to the mutability issues you've run into, but it also can make it hard to reason about what fields are populated when. Perhaps we could have something like a ColumnChunkIndex or something?

@liukun4515 liukun4515 force-pushed the parquet_column_index_write branch 2 times, most recently from cf7cbfc to ffa3d68 Compare June 29, 2022 14:35
@liukun4515 liukun4515 requested a review from tustvold June 29, 2022 14:41
@liukun4515
Copy link
Contributor Author

liukun4515 commented Jun 29, 2022

@tustvold reset, PTAL

tustvold
tustvold reviewed Jun 29, 2022
View reviewed changes
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it, some minor nits and then this can go in 😄

@tustvold tustvold added the api-change Changes to the arrow API label Jun 29, 2022
@liukun4515 liukun4515 force-pushed the parquet_column_index_write branch from ffa3d68 to a5faeb6 Compare June 30, 2022 01:17
Ted-Jiang
Ted-Jiang reviewed Jun 30, 2022
View reviewed changes
parquet/src/file/metadata.rs
null_pages: Vec<bool>,
min_values: Vec<Vec<u8>>,
max_values: Vec<Vec<u8>>,
// TODO: calc the order for all pages in this column
Copy link
Member

@Ted-Jiang Ted-Jiang Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this is useful for checking whether use page index

Copy link
Contributor Author

@liukun4515 liukun4515 Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this is useful for checking whether use page index

If the data in the pages are ordered by ascend or descend, we can use the binary search to accelerate the page filter.

Copy link
Contributor Author

@liukun4515 liukun4515 Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the boundaryorder is UNORDERED, we need to filter the page one by one.

@tustvold tustvold merged commit c47d14c into apache:master Jun 30, 2022
@tustvold
Copy link
Contributor

tustvold commented Jun 30, 2022

Thank you 🥇

@liukun4515 liukun4515 deleted the parquet_column_index_write branch June 30, 2022 11:27
@liukun4515 liukun4515 mentioned this pull request Jul 1, 2022
Closed
@alamb alamb changed the title add column index writer for parquet Add column index writer for parquet Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ted-Jiang Ted-Jiang Ted-Jiang left review comments

@tustvold tustvold tustvold approved these changes

@alamb alamb Awaiting requested review from alamb

@sunchao sunchao Awaiting requested review from sunchao

Assignees

No one assigned

Labels

api-change Changes to the arrow API parquet Changes to the parquet crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@liukun4515 @codecov-commenter @tustvold @Ted-Jiang