Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Integrate Record Skipping into Column Reader Fuzz Test #2315

Merged
merged 3 commits into from
Aug 7, 2022
Merged

Integrate Record Skipping into Column Reader Fuzz Test #2315

merged 3 commits into from
Aug 7, 2022

Conversation

Ted-Jiang
Copy link
Member

Which issue does this PR close?

Closes #2198.

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 Aug 4, 2022
@Ted-Jiang Ted-Jiang marked this pull request as draft August 4, 2022 05:44
@Ted-Jiang
Copy link
Member Author

Got 4 test error 😂 will dive into
image

@Ted-Jiang
Copy link
Member Author

Ted-Jiang commented Aug 4, 2022 •
edited
Loading

There are two kinds of erroe
one is like @thinkharderdev #2316 mentioned

failed to skip rows, expected x, got 4

one is parquet/src/arrow/arrow_reader.rs:1260

assert_eq!(a.data(), b.data(), "{:#?} vs {:#?}", a.data(), b.data());

@Ted-Jiang
Copy link
Member Author

Ted-Jiang commented Aug 4, 2022 •
edited
Loading

as test test_primitive_single_column_reader_test
it fail at Running type INT64 single_column_reader_test ConvertedType::NONE/ArrowType::None with Options: TestOptions { num_row_groups: 3, num_rows: 256, record_batch_size: 73, null_percent: None, write_batch_size: 64, max_data_page_size: 128, max_dict_page_size: 1048576, writer_version: PARQUET_1_0, enabled_statistics: Page, encoding: PLAIN, row_selections: Some(([RowSelection { row_count: 184, skip: false }, RowSelection { row_count: 184, skip: true }, RowSelection { row_count: 184, skip: false }, RowSelection { row_count: 184, skip: true }, RowSelection { row_count: 32, skip: false }], 400)) }i will check this first 🤔

@tustvold
Copy link
Contributor

tustvold commented Aug 4, 2022

Awesome! FWIW it might help debugging to seed the random number generator to get a deterministic test. I've found the fuzz tests are very good at finding bugs, but can be quite hard to then debug 😅

@Ted-Jiang
Copy link
Member Author

Awesome! FWIW it might help debugging to seed the random number generator to get a deterministic test. I've found the fuzz tests are very good at finding bugs, but can be quite hard to then debug 😅

Thanks ! i have found the combination args cause bug 😂

@liukun4515
Copy link
Contributor

ConvertedType::NONE/ArrowType::None

From the error message, I guess the error is related to the ConvertedType::NONE/ArrowType::None.

read/convert parquet data to arrow, we need the target arrow type and parquet logical type/convertedtype

@Ted-Jiang Ted-Jiang mentioned this pull request Aug 5, 2022
Closed
@Ted-Jiang Ted-Jiang marked this pull request as ready for review August 7, 2022 13:08
@Ted-Jiang
Copy link
Member Author

@tustvold seems all tests passed. I copy the all test case with page static. 😊

tustvold
tustvold approved these changes Aug 7, 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.

Very nice 👍

parquet/src/arrow/arrow_reader.rs
let mut skip_data: Vec<Option<T::T>> = vec![];
for select in selections {
if select.skip {
without_skip_data.drain(0..select.row_count);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👌

parquet/src/arrow/arrow_reader.rs
let batch = maybe_batch.unwrap().unwrap();
assert_eq!(end - total_read, batch.num_rows());
//TODO remove this after implement https://github.com/apache/arrow-rs/issues/2197
Copy link
Contributor

Choose a reason for hiding this comment

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

The linked issue has been done?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, maybe merge this and use this test to check the result

@tustvold tustvold merged commit 5676c6e into apache:master Aug 7, 2022
@ursabot
Copy link

ursabot commented Aug 7, 2022

Benchmark runs are scheduled for baseline = 38764c2 and contender = 5676c6e. 5676c6e 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

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

@tustvold tustvold tustvold approved these changes

Assignees
No one assigned
Labels
parquet Changes to the parquet crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate Record Skipping into Column Reader Fuzz Tests
4 participants
@Ted-Jiang @tustvold @liukun4515 @ursabot