Skip to content

feat(model): add data product concept and registry - #1001

Open
marcpaterno wants to merge 1 commit into
stack/concept-registry/02-empty-valuesfrom
stack/concept-registry/03-concept-registry
Open

marcpaterno wants to merge 1 commit into
stack/concept-registry/02-empty-valuesfrom
stack/concept-registry/03-concept-registry

Conversation

@marcpaterno

Copy link
Copy Markdown
Member

Summary

PR 3 of the five-PR concept-registry stack; review against
stack/concept-registry/02-empty-values.

Add experimental data-product concepts: named abstractions that can be modeled
by multiple concrete data-product types. Add a registry owning those concepts.

data_product_concept
  non-empty name + set of concrete types (std::type_index)

registry[name]
  register new concept -> take ownership, return true
  register same name  -> union concrete types, return false
  add concrete type  -> create concept if absent
  query              -> name lookup, sorted names, membership, reverse lookup
  • Reject empty concept names and null concept registration; type insertion is
    idempotent. Concept equality compares name and concrete-type membership.
  • Permit a concrete type to model more than one concept. Reverse lookup returns
    a matching concept, not a deterministic preferred concept.
  • Protect registry reads/writes with shared/exclusive locks; the registry is
    neither copyable nor movable.
  • Add model exports, forward declarations, build entries, membership/registry
    tests, and the explicit <mutex> include required for portability.

Conversion records and executable translator nodes belong to subsequent PRs.

Evidence

  • Before: The parent has no data-product concept or owning registry API.
    After: Membership tests pass 16 assertions in 8 cases; registry tests pass
    61 assertions in 22 cases, covering insertion, duplicate union, lookups,
    rejection, sorted names, and shared-type membership.
  • Full configured build, including Python and FORM targets, passed.
  • Full boundary suite: 119/119 CTests passed, adding exactly the two model tests.
  • All nine changed files passed existing hooks and whitespace checks.

The before/after is the API delta, not a failing-parent test claim. Results are
local macOS/AppleClang; Linux and TSAN execution are not established. Registry
cases are sequential: overlapping-operation runtime verification is still open
as separate follow-up work.

Merge Danger

Door: Two-way

Additive experimental model APIs with no persisted-format change. Reversion
after later adoption requires removing dependent APIs and callers.

Blast Radius: Model

Locks protect operations through registry methods, not dereferences or mutations
of returned pointers after a call. Callers must coordinate those accesses.
Mutable lookup also permits assignment that changes a concept's identity without
updating its registry key; that inherited API limitation is not hardened here.


Stack created with GitHub Stacks CLI • Give Feedback 💬

@coderabbitai

coderabbitai Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: e979aafc-2e1d-4b6c-bf02-864632d42d99
📥 Commits

Reviewing files that changed from the base of the PR and between cf90797 and 6e639c9.

📒 Files selected for processing: 9
  • phlex/model/CMakeLists.txt
  • phlex/model/data_product_concept.cpp
  • phlex/model/data_product_concept.hpp
  • phlex/model/data_product_concept_registry.cpp
  • phlex/model/data_product_concept_registry.hpp
  • phlex/model/fwd.hpp
  • test/CMakeLists.txt
  • test/data_product_concept_registry_test.cpp
  • test/data_product_concept_test.cpp
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@marcpaterno
marcpaterno added this pull request to stack #1004 October 21, 2026 17:45
@marcpaterno
marcpaterno requested a review from knoepfel October 21, 2026 17:56
@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Clang-Tidy Check Results

Clang-tidy found the following issue(s) on lines changed by this pull request:

/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept.cpp:8: warning: no header providing "std::string" is directly included
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept.cpp:8: warning: no header providing "std::move" is directly included
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept.cpp:17: warning: no header providing "std::unordered_set" is directly included
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept.cpp:36: warning: use 'contains' to check for membership
/__w/phlex/phlex/phlex-src/test/data_product_concept_test.cpp:24: warning: variable 'con' of type 'data_product_concept' can be declared 'const'
/__w/phlex/phlex/phlex-src/test/data_product_concept_test.cpp:30: warning: variable 'con' of type 'data_product_concept' can be declared 'const'
/__w/phlex/phlex/phlex-src/test/data_product_concept_registry_test.cpp:25: warning: variable 'registry' of type 'data_product_concept_registry' can be declared 'const'
/__w/phlex/phlex/phlex-src/test/data_product_concept_registry_test.cpp:34: warning: Moved-from object 'con' is moved
/__w/phlex/phlex/phlex-src/test/data_product_concept_registry_test.cpp:62: warning: no header providing "std::invalid_argument" is directly included
/__w/phlex/phlex/phlex-src/test/data_product_concept_registry_test.cpp:239: warning: variable 'registry' of type 'data_product_concept_registry' can be declared 'const'
/__w/phlex/phlex/phlex-src/test/data_product_concept_registry_test.cpp:271: warning: variable 'registry' of type 'data_product_concept_registry' can be declared 'const'
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept_registry.cpp:10: warning: no header providing "std::unique_ptr" is directly included
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept_registry.cpp:17: warning: variable 'lock' of type 'std::unique_lock<std::shared_mutex>' can be declared 'const'
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept_registry.cpp:26: warning: no header providing "std::string" is directly included
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept_registry.cpp:29: warning: variable 'lock' of type 'std::unique_lock<std::shared_mutex>' can be declared 'const'
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept_registry.cpp:35: warning: no header providing "std::make_unique" is directly included
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept_registry.cpp:54: warning: variable 'lock' of type 'std::shared_lock<std::shared_mutex>' can be declared 'const'
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept_registry.cpp:54: warning: no header providing "std::shared_lock" is directly included
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept_registry.cpp:60: warning: variable 'lock' of type 'std::shared_lock<std::shared_mutex>' can be declared 'const'
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept_registry.cpp:64: warning: no header providing "std::vector" is directly included
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept_registry.cpp:66: warning: variable 'lock' of type 'std::shared_lock<std::shared_mutex>' can be declared 'const'
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept_registry.cpp:72: warning: use a ranges version of this algorithm
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept_registry.cpp:79: warning: variable 'lock' of type 'std::shared_lock<std::shared_mutex>' can be declared 'const'
/__w/phlex/phlex/phlex-src/phlex/model/data_product_concept_registry.cpp:90: warning: variable 'lock' of type 'std::shared_lock<std::shared_mutex>' can be declared 'const'

Download clang-tidy artifacts

@codecov

codecov Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
phlex/model/data_product_concept_registry.cpp 97.61% 0 Missing and 1 partial ⚠️
@@                            Coverage Diff                             @@
##           stack/concept-registry/02-empty-values    #1001      +/-   ##
==========================================================================
+ Coverage                                   89.28%   89.37%   +0.09%     
==========================================================================
  Files                                         176      180       +4     
  Lines                                        6092     6145      +53     
  Branches                                     1016     1025       +9     
==========================================================================
+ Hits                                         5439     5492      +53     
  Misses                                        408      408              
  Partials                                      245      245              
Flag Coverage Δ
amd64 89.37% <98.11%> (+0.09%) ⬆️
arm64 89.37% <98.11%> (+0.09%) ⬆️

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

Files with missing lines Coverage Δ
phlex/model/data_product_concept.cpp 100.00% <100.00%> (ø)
phlex/model/data_product_concept.hpp 100.00% <100.00%> (ø)
phlex/model/data_product_concept_registry.hpp 100.00% <100.00%> (ø)
phlex/model/data_product_concept_registry.cpp 97.61% <97.61%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Harness.

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

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

@marcpaterno
marcpaterno force-pushed the stack/concept-registry/03-concept-registry branch from e515c6d to 6e639c9 Compare October 22, 2026 15:46
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.

1 participant