Skip to content

Conversation

@wgtmac
Copy link
Member

@wgtmac wgtmac commented Nov 27, 2022

This is the first step to support page index of parquet.

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@wgtmac
Copy link
Member Author

wgtmac commented Nov 27, 2022

@pitrou @emkornfield Can you please take a look? As I have mentioned in other thread, I will work on the page index and break it down into small commits to make it review-friendly.

@wgtmac wgtmac changed the title ARROW-10158: [C++][Parquet] Expose page index info from ColumnChunkMetaData ARROW-18413: [C++][Parquet] Expose page index info from ColumnChunkMetaData Nov 29, 2022
@github-actions
Copy link

std::unique_ptr<ColumnCryptoMetaData> crypto_metadata() const;

bool has_column_index() const;
int64_t column_index_offset() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be a clearer contract to have something like:

struct IndexLocation {
   int64_t index_file_offset_bytes;
   int32_t offset_index_length
}

optional<IndexLocation> GetColumIndexLocation();
optional<IndexLocation> GetOffsetIndexLocation();

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

SGTM, I will change it shortly.

@wgtmac
Copy link
Member Author

wgtmac commented Nov 30, 2022

I have addressed your comment, and the unsuccessful CI checks are unrelated to my change. Can you please take a look again? @emkornfield

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thank you. Is it possible to add basic tests for this?

@wgtmac
Copy link
Member Author

wgtmac commented Nov 30, 2022

Thank you. Is it possible to add basic tests for this?

Thank you for the review. @pitrou I have added a simple test and addressed your comment. Please take a look again when you get the chance.

@wgtmac wgtmac requested a review from pitrou December 1, 2022 06:53
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thanks for the update @wgtmac !

@pitrou
Copy link
Member

pitrou commented Dec 1, 2022

Test failures are unrelated, will merge.

@pitrou pitrou merged commit 958fbfa into apache:master Dec 1, 2022
@ursabot
Copy link

ursabot commented Dec 1, 2022

Benchmark runs are scheduled for baseline = 6f4a539 and contender = 958fbfa. 958fbfa is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.03% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.82% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.35% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 958fbfa5 ec2-t3-xlarge-us-east-2
[Failed] 958fbfa5 test-mac-arm
[Finished] 958fbfa5 ursa-i9-9960x
[Finished] 958fbfa5 ursa-thinkcentre-m75q
[Finished] 6f4a5393 ec2-t3-xlarge-us-east-2
[Finished] 6f4a5393 test-mac-arm
[Finished] 6f4a5393 ursa-i9-9960x
[Finished] 6f4a5393 ursa-thinkcentre-m75q
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants