Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Fixed filter of predicate with validity #653

Merged
merged 2 commits into from
Dec 2, 2021

Conversation

ritchie46
Copy link
Collaborator

The filter kernel took the values bitmap of the mask/filter, this works in most cases, as it often contains false/unset bits, but it doesn't have to.

This PR fixes the behavior and in case of null values it make sure that we first mask & validities before we filter.

@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #653 (ffbf82a) into main (8300684) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #653      +/-   ##
==========================================
+ Coverage   69.89%   69.91%   +0.02%     
==========================================
  Files         299      299              
  Lines       16634    16639       +5     
==========================================
+ Hits        11626    11633       +7     
+ Misses       5008     5006       -2     
Impacted Files Coverage Δ
src/compute/filter.rs 53.62% <100.00%> (+3.62%) ⬆️
src/bitmap/immutable.rs 86.48% <0.00%> (ø)
src/io/parquet/read/nested_utils.rs 78.43% <0.00%> (+0.98%) ⬆️
src/bitmap/utils/slice_iterator.rs 92.53% <0.00%> (+1.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8300684...ffbf82a. Read the comment docs.

@jorgecarleitao
Copy link
Owner

We will need to check that this is consistent for all cases and update the docs accordingly (there is a big "WARNING" on the docs atm about this)

@ritchie46
Copy link
Collaborator Author

ritchie46 commented Dec 2, 2021

We will need to check that this is consistent for all cases and update the docs accordingly (there is a big "WARNING" on the docs atm about this)

What cases do you mean? The validity can only have two states, those are added in the test.

I will update the docs, missed that UB, and just passed filters with null values. xD.

As this is the entrypoint on any filter. What this new behavior does, is changing the invalid array with nulls to a valid one without nulls, and this is expected of all downstream kernels atm.

@jorgecarleitao jorgecarleitao added the bug Something isn't working label Dec 2, 2021
@jorgecarleitao jorgecarleitao changed the title fix filter boolean array with masked out true bits Fixed filter of predicate with validity Dec 2, 2021
@jorgecarleitao jorgecarleitao merged commit a830b53 into jorgecarleitao:main Dec 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants