-
Notifications
You must be signed in to change notification settings - Fork 976
Add tests for BatchCoalescer::push_batch_with_filter
, fix bug
#7774
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
Conversation
60beecd
to
65604c5
Compare
@@ -456,6 +460,98 @@ mod tests { | |||
.run(); | |||
} | |||
|
|||
/// Coalesce multiple batches, 80k rows, with a 0.1% selectivity filter |
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.
The idea is to get filter coverage prior to writing some specialized code for it
BatchCoalescer::push_batch_with_filter
BatchCoalescer::push_batch_with_filter
, fix bug
use std::fmt::Debug; | ||
use std::sync::Arc; | ||
|
||
/// InProgressArray for [`PrimitiveArray`] | ||
#[derive(Debug)] | ||
pub(crate) struct InProgressPrimitiveArray<T: ArrowPrimitiveType> { | ||
/// Data type of the array | ||
data_type: DataType, |
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.
previously primitive arrays like TimestampNanosecond would lose the timezone information. This fixes the issue
Opened apache/arrow-js#205 for the failing test (passed after retry). |
Thank you @mbrobbel |
Thanks agin @mbrobbel |
Which issue does this PR close?
push_batch_with_filter
for primitive array #7762Rationale for this change
As part of #7762 I want to optimize applying filters by adding a new code path.
To ensure that works well, let's ensure the filtered code path is well covered with tests
What changes are included in this PR?
Are these changes tested?
Only tests, no functional changes
Are there any user-facing changes?