Add how-data-products-are-resolved.md#72
Conversation
📝 WalkthroughWalkthroughAdds a design wiki document describing how product selectors, layers, graph creation, runtime resolution, and ambiguous matches are handled for data products. ChangesData Product Resolution
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design_wiki/seeds/how-data-products-are-resolved.md`:
- Around line 1-3: Update the Markdown formatting in the documented headings and
list blocks, including the ordered list around Lines 37–42, by adding blank
lines before and after each heading and between surrounding prose and every
list. Ensure all affected sections satisfy Markdownlint rules MD022 and MD032
without changing their content.
- Around line 20-26: Rewrite the layer-matching guidance to state that
graph-time resolution generally ignores layer name, then explicitly document the
provider exceptions: provider-produced products match on layer name, and
explicit providers statically use their specified layer path during graph
creation. Ensure the rules clearly distinguish non-provider, provider, and
explicit-provider products.
- Around line 33-36: Clarify the status of the multi-edge selector behavior in
“Contentious Part” by marking it as a proposal, or align the canonical selector
contract in the related design documentation. If adopting the behavior,
explicitly document that a selector may resolve to multiple products and update
the graph-initialization error rule; otherwise retain the
single-match-per-selector rule here.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 56218e8c-39ff-47a3-bec1-ac9316752a16
📒 Files selected for processing (1)
design_wiki/seeds/how-data-products-are-resolved.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Respect markdownlint rules: MD012, MD022, MD031, MD032, MD034, MD040
Files:
design_wiki/seeds/how-data-products-are-resolved.md
**/*.{md,rst,qmd}
📄 CodeRabbit inference engine (AGENTS.md)
Ensure one trailing newline and no trailing whitespace in all documentation files
Files:
design_wiki/seeds/how-data-products-are-resolved.md
🪛 markdownlint-cli2 (0.23.0)
design_wiki/seeds/how-data-products-are-resolved.md
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
[warning] 2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 12-12: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 19-19: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 28-28: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 33-33: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
| ## How Data Products are Resolved | ||
| ### Product Selectors | ||
| - Data products are selected using a `product_selector` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add blank lines around headings and list blocks.
The headings and immediately following lists are not separated by blank lines, and the ordered list at Lines 37–42 also follows prose without a blank line. This will violate the repository’s Markdown lint rules.
As per coding guidelines, **/*.md must respect Markdownlint rules MD022 and MD032.
Also applies to: 12-13, 19-20, 28-29, 33-37
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
[warning] 2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design_wiki/seeds/how-data-products-are-resolved.md` around lines 1 - 3,
Update the Markdown formatting in the documented headings and list blocks,
including the ordered list around Lines 37–42, by adding blank lines before and
after each heading and between surrounding prose and every list. Ensure all
affected sections satisfy Markdownlint rules MD022 and MD032 without changing
their content.
Sources: Coding guidelines, Linters/SAST tools
| - All fields except layer name are used to resolve the data product at graph creation time | ||
| - This is because the layer name may not be constant across instances of the product | ||
| - Edges are created only from input products which match all specified fields of the selector | ||
| - Where the product is produced by a HOF other than a provider the relevant fields are: Concept, Creator name, and suffix | ||
| - Where the product is produced by a provider, the relevant fields are: Concept, creator name, suffix, and layer name | ||
| - Where the product is produced by an *explicit* provider, the layer name is also used at graph creation time | ||
| - Explicit providers statically specify the layer path associated with their output |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clarify the graph-time layer-matching rule.
Line 20 excludes layer name from graph-time resolution, but Lines 24–26 state that providers—especially explicit providers—use it at graph creation. Rewrite this as an explicit general rule with provider exceptions so implementers cannot construct the wrong graph.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design_wiki/seeds/how-data-products-are-resolved.md` around lines 20 - 26,
Rewrite the layer-matching guidance to state that graph-time resolution
generally ignores layer name, then explicitly document the provider exceptions:
provider-produced products match on layer name, and explicit providers
statically use their specified layer path during graph creation. Ensure the
rules clearly distinguish non-provider, provider, and explicit-provider
products.
| #### Contentious Part | ||
| - A selector may match multiple products when its layer name is ignored, while only matching a single product in any given data cell | ||
| - This requires allowing the creation of multiple edges for the same input port | ||
| - We must nevertheless prevent duplicate matches within a single data cell |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== design_wiki/seeds/how-data-products-are-resolved.md (around lines 33-36) ==\n'
sed -n '1,120p' design_wiki/seeds/how-data-products-are-resolved.md | nl -ba | sed -n '25,45p'
printf '\n== doc/ch_technical_design/data_product_management.rst (around lines 48-59) ==\n'
sed -n '1,120p' doc/ch_technical_design/data_product_management.rst | nl -ba | sed -n '40,70p'
printf '\n== search for selector/multi-match language ==\n'
rg -n "multiple data products|multiple products|selector|input port|graph-initialization error|initialization error|duplicate matches" design_wiki doc -SRepository: Framework-R-D/phlex-design
Length of output: 284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== design_wiki/seeds/how-data-products-are-resolved.md (around lines 33-36) ==\n'
awk 'NR>=25 && NR<=45 { printf "%4d %s\n", NR, $0 }' design_wiki/seeds/how-data-products-are-resolved.md
printf '\n== doc/ch_technical_design/data_product_management.rst (around lines 48-59) ==\n'
awk 'NR>=40 && NR<=70 { printf "%4d %s\n", NR, $0 }' doc/ch_technical_design/data_product_management.rst
printf '\n== search for selector/multi-match language ==\n'
rg -n "multiple data products|multiple products|selector|input port|graph-initialization error|initialization error|duplicate matches" design_wiki doc -SRepository: Framework-R-D/phlex-design
Length of output: 12551
Mark this as a proposal, or align the canonical selector contract. doc/ch_technical_design/data_product_management.rst still says resolving a product_selector to multiple data products is a graph-init error, while this section introduces multi-edge resolution for one input port. If this is a behavior change, call it out explicitly and update the canonical design; otherwise keep the single-match rule here.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 33-33: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design_wiki/seeds/how-data-products-are-resolved.md` around lines 33 - 36,
Clarify the status of the multi-edge selector behavior in “Contentious Part” by
marking it as a proposal, or align the canonical selector contract in the
related design documentation. If adopting the behavior, explicitly document that
a selector may resolve to multiple products and update the graph-initialization
error rule; otherwise retain the single-match-per-selector rule here.
| - The "layer name" is the name of the lowest ("leaf") layer in the path | ||
| - Each instance of a data product is placed in a data cell | ||
| - Each instance of a data product is therefore transitively associated with a layer path, and layer name | ||
| - These do not have to be constant across instances of a data product |
There was a problem hiding this comment.
I would word this differently. Perhaps: "A product_selector instance may select data products that do not all have the same data layer."
Once assigned to a data product, the data layer is constant.
| ### Layers Checking / Resolution | ||
| - It is desirable that a CHOF be able to take a given input product from different layers on different iterations | ||
| - On each iteration the output layer of the CHOF is determined by the layers of its inputs | ||
| - Therefore layer name checking must be delayed til runtime, when data flows along the created edges |
There was a problem hiding this comment.
I think the idea of layer-checking needs to be fleshed out a little. For example, at what points in the execution of a single graph iteration does layer-checking need to happen (and why)? Right now I can only think of two scenarios:
- Routing—the initial sending of the data-cell index to the correct providers, which requires inspecting the layer of the data-cell index (currently achieved by the
index_router). - Layer filtering—when a CHOF with at least one input product selector having a specified
layerfield is downstream of a CHOF that does not have a specifiedlayerfield in its input product selectors.
Am I missing other scenarios?
2 participants
how-data-products-are-resolved.mdto the design wiki seeds.product_selectorbehavior.