FIX-#3527: Fix parquet partitioning issue causing negative row length partitions #4368
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
|
|
Codecov Report
@@ Coverage Diff @@
## master #4368 +/- ##
===========================================
+ Coverage 70.79% 90.01% +19.22%
===========================================
Files 219 219
Lines 17944 17946 +2
===========================================
+ Hits 12703 16154 +3451
+ Misses 5241 1792 -3449
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
jeffreykennethli
added
bug 🦗
mvashishtha
reviewed
Apr 7, 2022
modin/pandas/test/test_io.py
Outdated
| unique_filename = get_unique_filename(extension="parquet") | ||
| from modin.config import NPartitions | ||
|
|
||
| nrows = (NPartitions.get() - 1) * 32 - 1 |
There was a problem hiding this comment.
Could you please:
- Use
MinPartitionSizeinstead of 32 - add an assertion that
nrows > MinPartitionSize.get()? If at some point in the future,NPartitionsbecomes 2, this will test the case wherenrows = 31 < min_chunk_size = 32instead of the one you're trying to test.
There was a problem hiding this comment.
I changed it to use MinPartitionSize. I also removed the dependency on NPartitions, since MinPartitionSize + 1 is a more robust failure case i.e. is expected to fail (without these changes) regardless of the value of MinPartitionSize or NPartitions.
jeffreykennethli
commented
Apr 8, 2022
There was a problem hiding this comment.
@jeffreykennethli Looks great! The only thing left is to add this to the release notes (and add your github handle to the contributors section of that file
jeffreykennethli
force-pushed
the
parquet_read
branch
from
160508d
to
72e4d41
Compare
YarShev
reviewed
Apr 12, 2022
YarShev
reviewed
Apr 12, 2022
jeffreykennethli
force-pushed
the
parquet_read
branch
from
d6926fb
to
2b64072
Compare
|
@jeffreykennethli, please resolve conflicts. After that we will merge the changes. |
jeffreykennethli
force-pushed
the
parquet_read
branch
2 times, most recently
from
a43a10a
to
d03082c
Compare
YarShev
reviewed
Apr 19, 2022
added 8 commits
April 19, 2022 14:16
Signed-off-by: jeffreykennethli <jkli@ponder.io>
…lumn Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
jeffreykennethli
force-pushed
the
parquet_read
branch
from
237350d
to
4d54048
Compare
|
@YarShev any other comments? |
devin-petersohn
pushed a commit
that referenced
this pull request
May 4, 2022
… partitions (#4368) Signed-off-by: jeffreykennethli <jkli@ponder.io>
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.
What do these changes do?
In #3527 , we found that partitions from
read_parquetwould have negative_length_cache, giving aValueError: lengths cannot be negativewhen trying to materialize results, such as when indexing into a DataFrame by column name._length_cacheis set when the DataFrame is initialized and partitioned. For parquet files, this is in the column_store_dispatcher.The previous logic would partition the row lengths with a minimum chunk size (32 rows). For DataFrames with less than 32 total rows, it would partition the rows:
[32, 0, 0, .... , 0]for each of the N partitions.For DataFrames with more than
num_partitions * min_chunk_sizerows i.e.index_len >= min_chunk_size * num_partitions, it would be partitioned evenly[index_len / num_partitions, index_len / num_partitions, ...].For DataFrames where
min_chunk_size < index_len < min_chunk_size * num_partitions, it would partition it like[32, 32, 32, ... index_len - (index_chunksize * (num_partitions - 1)]and theindex_len - (index_chunksize * (num_partitions - 1)expression would be negative. This ensured the sum of the row lengths of all partitions would be equal toindex_lenbut the negative row lengths is wrong.It should be partitioned like
[32, 32, 32, 0, 0, ...]where the firstmrows aremin_chunk_sizeuntil the partitions have more cumulative rows thanindex_len, and be 0 for every partition after.This is the change this PR is introducing, as well as adding a error checking statement in the DataFrame constructor to check that no partitions have negative row length or column width, and a unit test that checks for indexing into all columns of a parquet file with various row lengths. I think there is room to write a more appropriate unit test, as this one minimally tests this broken behavior.
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.pyblack --check modin/ asv_bench/benchmarks scripts/doc_checker.pygit commit -sdocs/development/architecture.rstis up-to-date