-
Notifications
You must be signed in to change notification settings - Fork 855
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
Support empty projection in ParquetRecordBatchReader
#1560
Conversation
@@ -214,12 +205,6 @@ impl ParquetRecordBatchReader { | |||
batch_size: usize, | |||
array_reader: Box<dyn ArrayReader>, | |||
) -> Result<Self> { | |||
// Check that array reader is struct array reader | |||
array_reader |
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.
This check does not seem necessary, all it cares about is that it yields StructArray
/// but with row counts that correspond to the amount of data in the file | ||
/// | ||
/// This is useful for when projection eliminates all columns within a collection | ||
pub fn make_empty_array_reader(row_count: usize) -> Box<dyn ArrayReader> { |
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.
I don't like this name, but it was the best I could come up with
} | ||
|
||
struct EmptyArrayReader { | ||
data_type: ArrowType, |
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.
data_type
seems not necessary to keep here, it is always ArrowType::Struct(vec![])
.
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.
It's needed for get_data_type(&self) -> &ArrowType
as it needs to return a reference with a lifetime coupled to the object itself.
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.
yea, just found it and want to go to comment. you already replied. nvm.
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.
lgtm, thanks @tustvold
Codecov Report
@@ Coverage Diff @@
## master #1560 +/- ##
==========================================
- Coverage 82.86% 82.85% -0.01%
==========================================
Files 190 191 +1
Lines 55057 55085 +28
==========================================
+ Hits 45622 45642 +20
- Misses 9435 9443 +8
Continue to review full report at Codecov.
|
ParquetRecordBatchReader
Which issue does this PR close?
Closes #1537
Rationale for this change
See ticket
What changes are included in this PR?
See ticket
Are there any user-facing changes?
No, the only changes are to experimental modules.