Skip to content

Do not prematurely mark filter as ready for downstream processing#158

Merged
sabasehrish merged 2 commits intoFramework-R-D:mainfrom
knoepfel:fix-filtering
Dec 4, 2025
Merged

Do not prematurely mark filter as ready for downstream processing#158
sabasehrish merged 2 commits intoFramework-R-D:mainfrom
knoepfel:fix-filtering

Conversation

@knoepfel
Copy link
Copy Markdown
Member

@knoepfel knoepfel commented Dec 3, 2025

Phlex supports conditional execution of nodes through when("predicate1", "predicate2", ...) clauses, which can be included as part of a registration statement. When this occurs, that node is not allowed to execute until:

  1. All upstream data products required by that node are available
  2. All predicates ("predicate1", "predicate2", etc.) have been evaluated to true

This PR fixes a problem with 1 above—namely, it makes a change to ensure that the products (actually, the product stores containing the products) are non-null before marking the filter as ready for downstream processing.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a critical bug in the conditional execution system where filters could be prematurely marked as ready for downstream processing. The issue occurred when product stores in the filter's data map were null, but the vector still had the expected size. The fix ensures that only non-null product stores are counted when determining if a filter has all required upstream data products available.

  • Changed data_map::is_complete() to count only non-null product stores instead of just checking vector size
  • Added assertions in more_derived() to catch null stores early and fail fast

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
phlex/core/detail/filter_impl.cpp Modified is_complete() to use std::ranges::count_if to count only non-null stores; added <algorithm> include for the ranges algorithm
phlex/core/message.cpp Added defensive assertions to verify product stores are non-null before accessing their depth in more_derived()

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##             main     #158      +/-   ##
==========================================
+ Coverage   81.14%   81.23%   +0.08%     
==========================================
  Files         116      116              
  Lines        2042     2046       +4     
  Branches      326      328       +2     
==========================================
+ Hits         1657     1662       +5     
  Misses        250      250              
+ Partials      135      134       -1     
Flag Coverage Δ
unittests 81.23% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
phlex/core/detail/filter_impl.cpp 95.23% <100.00%> (+2.73%) ⬆️
phlex/core/message.cpp 61.53% <100.00%> (+6.99%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@sabasehrish sabasehrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good.

@sabasehrish sabasehrish merged commit 2994c8a into Framework-R-D:main Dec 4, 2025
36 checks passed
@knoepfel knoepfel deleted the fix-filtering branch December 4, 2025 15:49
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.

3 participants