-
Notifications
You must be signed in to change notification settings - Fork 232
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.
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
Fix issue with parquet partitioned reads #741
Conversation
Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
build |
.createPartitionValues(partitionValues, partitionSchema) | ||
withResource(partitionScalars) { scalars => | ||
ColumnarPartitionReaderWithPartitionValues.addPartitionValues(cb, scalars) | ||
if (partitionSchema.nonEmpty) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this change is not needed I originally started it to only grab the semaphore when there were partition values, but then I thought better and decided to grab it in all cases. I left this because I thought it made the code more readable, even though it works without the change.
If others disagree I am happy to remove it.
python udf window tests are failing here too now. Not sure how they passed for the original PR. Because this is a P1 I am going to merge this in as is, and then we can decide how to fix the test separately. |
Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
[auto-merge] bot-auto-merge-branch-22.12 to branch-23.02 [skip ci] [bot]
This fixes #718
It also fixes a few other things I found while in the code.
It acquires the GPU Semaphore in cases where we are returning a batch, even if it is just row counts.
It updates the tests to use the new small file config. I noticed it when I checked if my new test would fail without the fix in place, and it failed all 4 cases instead of just half of them that I thought it should.