Skip to content

Conversation

@fatemehp
Copy link
Contributor

@fatemehp fatemehp commented Oct 10, 2022

Add a test for TypedColumnReader::Skip with repeated values to make it clear that we are skipping values and not records.
Also, add some comments to the existing test for Skip of non-repeated values.

@github-actions
Copy link

@github-actions
Copy link

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

@fatemehp fatemehp changed the base branch from master to decimal256 October 10, 2022 17:59
@fatemehp fatemehp changed the base branch from decimal256 to master October 10, 2022 18:00
@fatemehp fatemehp marked this pull request as ready for review October 10, 2022 18:05
@fatemehp
Copy link
Contributor Author

@emkornfield can you merge this pull request? You already reviewed it, but the pull request was not against the master. This includes some commits that are already checked in. I am not sure how to remove them. You can try looking at the changes from "Merge pull request #1 from fatemehp/skip_test".

@kou kou changed the title PARQUET-2179: [parquet-cpp] Add a test for skipping repeated fields PARQUET-2179: [C++][Parquet] Add a test for skipping repeated fields Oct 10, 2022
@pitrou
Copy link
Member

pitrou commented Oct 11, 2022

@fatemehp Why did you submit this in addition to #14142?

@emkornfield
Copy link
Contributor

@fatemehp it looks like this merged commits from the SkipRecords with the additional tests for Skip method that only skips values, could you remove some of the commits?

@fatemehp
Copy link
Contributor Author

This is a test for TypedColumnReader::Skip, and not relevant to the RecordReader. I am trying to keep the changes small and self-contained.

@fatemehp
Copy link
Contributor Author

@emkornfield yes, there are some commits here that are not part of this change. I will remove them.

@fatemehp
Copy link
Contributor Author

Ok, I think I have fixed this. @emkornfield, could you take a look and merge this change if everything looks good? Thanks!

@pitrou pitrou self-requested a review October 17, 2022 20:02
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.

+1, thank you @fatemehp

@pitrou
Copy link
Member

pitrou commented Oct 18, 2022

Let's wait for CI before merging this.

@pitrou
Copy link
Member

pitrou commented Oct 18, 2022

CI failures are unrelated.

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.

3 participants