Skip to content
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(filters): new filterPredicate shouldn't break other column filters #1531

Merged
merged 2 commits into from
May 16, 2024

Conversation

ghiscoding
Copy link
Owner

@ghiscoding ghiscoding commented May 16, 2024

  • the previous PR to implement filterPredicate regressed with a new issue and that was not caught in the original PR. It had the indirect effect of breaking the other filter columns (I forgot to test that in the original PR), the issue was caused by the fact that calling a return within the for loop of all filters was cancelling all other filters in the loop because a return breaks the entire loop.
  • So the fix is to only call return when the filterPredicate returns false which mean that at point the row data context is officially filtered out and so stopping inspection of further filters does make sense at that point in time.
  • also improve SQL LIKE with filter of Ta%30%, it wasn't working correctly before and it is now equivalent to: StartsWith "Ta" and Contains "30" anywhere

below with the fix, other column filters now work (before the fix, the 2nd column filter wasn't doing anything)

msedge_bbEm4vYgpN

- the previous PR to implement `filterPredicate` that was not caught originally which had the indirect effect of breaking the other filter columns, the issue was caused by the fact that calling a `return` within the `for` loop of all filters was cancelling the next filters because a `return` breaks the loop.
- So the fix is to only call `return` when the `filterPredicate` returns `false` which mean that at point the row data context is officially filtered out, so not inspecting further filters make sense
Copy link

stackblitz bot commented May 16, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

codecov bot commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (828eb8a) to head (f7ce7a6).

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1531     +/-   ##
========================================
+ Coverage    99.8%   99.8%   +0.1%     
========================================
  Files         198     198             
  Lines       21621   21623      +2     
  Branches     6951    7222    +271     
========================================
+ Hits        21560   21562      +2     
+ Misses         61      55      -6     
- Partials        0       6      +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- `Ta%30%` wasn't working correctly before, it should be equivalent to: StartsWith "Ta" and Contains "30" anywhere
@ghiscoding ghiscoding merged commit 27777ef into master May 16, 2024
7 checks passed
@ghiscoding ghiscoding deleted the bugfix/filter-predicate-loop branch May 16, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants