Disable Parallel Parquet Writer by Default, Improve Writing Test Coverage #8854
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
devinjdangelo
commented
Jan 15, 2024
| @@ -403,7 +403,7 @@ config_namespace! { | |||
| /// parquet files by serializing them in parallel. Each column | |||
| /// in each row group in each output file are serialized in parallel | |||
| /// leveraging a maximum possible core count of n_files*n_row_groups*n_columns. | |||
| pub allow_single_file_parallelism: bool, default = true | |||
| pub allow_single_file_parallelism: bool, default = false | |||
| @@ -64,6 +64,25 @@ select * from validate_parquet; | |||
| 1 Foo | |||
| 2 Bar | |||
|
|
|||
| query ?? | |||
| COPY | |||
| (values (struct ('foo', (struct ('foo', make_array(1,2,3)))), make_array(timestamp '2023-01-01 01:00:01',timestamp '2023-01-01 01:00:01')), | |||
There was a problem hiding this comment.
Thank you. I left a comment there
Would it be possible to change the constant values of the two rows so it is clear they are different
e.g.
Suggested change
| (values (struct ('foo', (struct ('foo', make_array(1,2,3)))), make_array(timestamp '2023-01-01 01:00:01',timestamp '2023-01-01 01:00:01')), | |
| (values (struct ('foo', (struct ('foo', make_array(10,20,30)))), make_array(timestamp '2024-01-01 01:00:01',timestamp '2024-01-01 01:00:01')), |
There was a problem hiding this comment.
I pushed up a change to make the rows distinct and add an array of struct as #8867 is fixed after pulling main.
9 tasks
alamb
reviewed
Jan 15, 2024
There was a problem hiding this comment.
Thanks @devinjdangelo -- adding tests is great and very much appreciated.
I think we can merge this PR as is, but if we can improve the tests slightly before doing so that would be great.
| @@ -64,6 +64,25 @@ select * from validate_parquet; | |||
| 1 Foo | |||
| 2 Bar | |||
|
|
|||
| query ?? | |||
| COPY | |||
| (values (struct ('foo', (struct ('foo', make_array(1,2,3)))), make_array(timestamp '2023-01-01 01:00:01',timestamp '2023-01-01 01:00:01')), | |||
There was a problem hiding this comment.
Thank you. I left a comment there
Would it be possible to change the constant values of the two rows so it is clear they are different
e.g.
Suggested change
| (values (struct ('foo', (struct ('foo', make_array(1,2,3)))), make_array(timestamp '2023-01-01 01:00:01',timestamp '2023-01-01 01:00:01')), | |
| (values (struct ('foo', (struct ('foo', make_array(10,20,30)))), make_array(timestamp '2024-01-01 01:00:01',timestamp '2024-01-01 01:00:01')), |
alamb
approved these changes
Jan 15, 2024
alamb
approved these changes
Jan 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Related to #8853 and #8851
Rationale for this change
A number of issues were recently brought up related to the parallel parquet writer. Most appear to be related to how nested types are being handled. It is not immediately obvious where the flaw is related to handling nested types.
What changes are included in this PR?
Disables parallel writing by default. Adds tests to
copy.sltthat cover the failure cases.Are these changes tested?
Yes
Are there any user-facing changes?
Potentially fewer cores will be used by default when writing parquet files.