feat(semantic): field catalog — types from ClickHouse + authored roles (#2208) - #2211
Closed
cyberantonz wants to merge 2 commits into
Closed
feat(semantic): field catalog — types from ClickHouse + authored roles (#2208)#2211cyberantonz wants to merge 2 commits into
cyberantonz wants to merge 2 commits into
Conversation
constructorfabric#2208) Phase 1 slice 2 of the semantic-layer definition core: the field catalog, the typed, role-annotated validation universe measures are checked against. Capability becomes a projection of definitions, not of data. The dbt schema.yml files carry no meta: type/role annotations, so the catalog source is hybrid (per the chosen approach): - roles.yaml — authored semantic roles (tenant/entity/event_time/ dimension/value/subject) per exposed field. - types.snapshot.json — column types generated from ClickHouse system.columns, committed so build-time validation needs no live warehouse, and kept honest by an ignored drift test against live CH. domain/field_catalog/ (Rust) joins the two into a typed FieldCatalog, rejecting any role that names an absent column or an unmodeled ClickHouse type — so the embedded catalog is a consistent validation universe at build time. Offline consistency tests gate it in normal CI; the CH drift test runs where a warehouse is available. `analytics field-catalog` prints it. Seed datasets: the two git relations that carry a direct entity (class_git_commits, class_git_pull_requests). Relations without a self-contained entity are derived-dataset cases catalogued during extraction (slice 5). Validated: offline tests green; drift test passes against live ClickHouse 25.7. Extends the semantic-layer DESIGN field-catalog component with slice-2 status, recording the hybrid-source deviation. Part of constructorfabric#2208 Part of constructorfabric#1803 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
📝 WalkthroughWalkthroughThis PR adds a Rust semantic-layer field catalog for 12 silver datasets. It combines authored field roles with ClickHouse type snapshots, validates catalog consistency, supports offline CLI rendering, documents Phase 1 coverage, and adds an ignored live schema-drift test. ChangesField catalog
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
6 tasks
…bric#2208) Address the "why only git" gap: the catalog was under-scoped to two git relations. Extend coverage to every source family with a clean, self-contained entity + tenant at silver — git, ai, collab, wiki (12 datasets) — and record what genuinely can't be catalogued without the extraction analysis. - Add FieldType::Decimal: collab/ai amount columns are Decimal(p,s), kept distinct from Float for exact currency arithmetic. - Author roles for ai (4), collab (4), wiki (2) relations; regenerate the ClickHouse type snapshot for all 12 relations (245 columns). - Tenant column name is not uniform across families (tenant_id vs insight_tenant_id) — named per dataset in roles.yaml. Deferred to extraction (slice 5), where roles are verified against real measures: task (no tenant column, entity is author_id not email — needs identity resolution), reference/join relations with no direct entity, and derived populations (active-day, focus). Validated: offline tests green (incl. Decimal normalization); drift test passes against live ClickHouse 25.7 for all 12 relations. DESIGN field-catalog note updated with coverage + deferrals. Part of constructorfabric#2208 Part of constructorfabric#1803 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/backend/services/analytics/src/domain/field_catalog/mod.rs (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove narrative comments from the service source.
These comments duplicate context that code, tests, CLI metadata, or design documentation should express.
src/backend/services/analytics/src/domain/field_catalog/mod.rs#L1-L4: remove the module header.src/backend/services/analytics/src/domain/field_catalog/mod.rs#L13-L14: remove therenderdocumentation comment.src/backend/services/analytics/src/main.rs#L93-L95: replace the documentation comment with the equivalent Clap help attribute.src/backend/services/analytics/src/main.rs#L121-L121: remove the comment. It refers toprint_field_catalog, but this branch callsrenderdirectly.src/backend/services/analytics/src/main.rs#L147-L148: remove the test documentation comment.src/backend/services/analytics/src/domain/field_catalog/live_tests.rs#L1-L9: remove the module header. Move test invocation instructions to design documentation if they must remain.As per coding guidelines, “Use comments only when code cannot express the reason,” and “Do not use module headers.”
🤖 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 `@src/backend/services/analytics/src/domain/field_catalog/mod.rs` around lines 1 - 4, Remove narrative comments at src/backend/services/analytics/src/domain/field_catalog/mod.rs:1-4 and :13-14, src/backend/services/analytics/src/main.rs:121-121 and :147-148, and src/backend/services/analytics/src/domain/field_catalog/live_tests.rs:1-9; replace the documentation comment at src/backend/services/analytics/src/main.rs:93-95 with the equivalent Clap help attribute, preserving the existing CLI help text. The main.rs:121 comment should be removed because the branch invokes render directly, and move live-test invocation guidance to design documentation if it must remain.Source: Coding guidelines
src/backend/services/analytics/src/domain/mod.rs (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestrict the module to crate visibility.
Use
pub(crate) mod field_catalog;unless an external crate imports this module. This reduces the public API surface without limiting internal callers.As per coding guidelines, “Use the smallest visibility that compiles: prefer
pub(crate)beforepub.”🤖 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 `@src/backend/services/analytics/src/domain/mod.rs` at line 5, Change the field_catalog module declaration in mod.rs from public to crate-visible by using the smallest visibility that supports existing internal callers. Preserve external visibility only if another crate directly imports field_catalog.Source: Coding guidelines
src/backend/services/analytics/src/domain/field_catalog/loader.rs (1)
282-338: 📐 Maintainability & Code Quality | 🔵 TrivialUse a table-driven rejection test.
These tests repeat the same build-and-match structure for different invalid catalog cases. Put the cases in one table. Include a case label in each assertion. Use
type R = Result<(), Box<dyn Error>>for the test return type.As per coding guidelines, “Make tests read as specifications: use table-driven loops with per-case assertion messages” and “alias
type R = Result<(), Box<dyn Error>>to reduce ceremony.”
[low_effort_and-high_reward]🤖 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 `@src/backend/services/analytics/src/domain/field_catalog/loader.rs` around lines 282 - 338, Consolidate the four rejection tests around build into one table-driven test, preserving each existing invalid document/snapshot setup and expected CatalogError match. Define the test return alias as type R = Result<(), Box<dyn Error>>, iterate through labeled cases, and include each case label in assertion messages so failures identify the specific rejection scenario.Source: Coding guidelines
src/backend/services/analytics/src/domain/field_catalog/model.rs (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove service source commentary that the code can express.
These comments add API documentation and a module header in an analytics service. Keep durable design context in the design document. Express validation behavior with types and tests.
src/backend/services/analytics/src/domain/field_catalog/model.rs#L3-L4: remove the service-local documentation comments, including the similar comments on the adjacent model types.src/backend/services/analytics/src/domain/field_catalog/loader.rs#L1-L7: remove the module header and retain this context indocs/domain/semantic-layer/specs/DESIGN.md.src/backend/services/analytics/src/domain/field_catalog/loader.rs#L216-L217: remove the explanatory test comment and make the test name and assertions state the rule.As per coding guidelines, “Use comments only when code cannot express the reason” and “Do not use module headers.”
🤖 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 `@src/backend/services/analytics/src/domain/field_catalog/model.rs` around lines 3 - 4, Remove the service-local documentation comments from src/backend/services/analytics/src/domain/field_catalog/model.rs lines 3-4 and the adjacent model types, without changing code behavior. Remove the module header from src/backend/services/analytics/src/domain/field_catalog/loader.rs lines 1-7, retaining durable context only in the design document. At lines 216-217, remove the explanatory test comment and rename the test and strengthen its assertions so they clearly express the validation rule.Source: Coding guidelines
🤖 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 `@docs/domain/semantic-layer/specs/DESIGN.md`:
- Around line 285-287: Update the catalog description and responsibility scope
to match the implementation’s lazy OnceLock::get_or_init construction, using
roles.yaml for authored roles and types.snapshot.json for column types. Remove
or revise the conflicting claim that dbt schema.yml metadata is the source and
roles are never hand-maintained, while preserving the documented dataset
coverage and deferred responsibilities.
In `@src/backend/services/analytics/src/domain/field_catalog/live_tests.rs`:
- Around line 46-55: Bound the `system.columns` read in the live-schema test
around the `fetch_all::<ColumnRow>()` call, using the project’s established
query limit or streamed-read mechanism. Ensure relations exceeding the limit
produce a clear over-limit failure while preserving the existing `BTreeMap`
construction for valid responses.
In `@src/backend/services/analytics/src/domain/field_catalog/loader.rs`:
- Around line 27-34: Update field_catalog and the CATALOG initialization around
build(ROLES_YAML, TYPES_SNAPSHOT_JSON) to propagate a typed initialization
Result instead of calling expect or panicking. Store the Result in OnceLock or
otherwise return the fallible outcome to callers, and update affected call sites
to handle the initialization error before serving requests.
- Around line 125-146: Update the field-loading logic around the fields
collection to track names already encountered within each dataset, and reject
repeats before adding a Field. Return the typed CatalogError::DuplicateField for
duplicate dimension or value names, while preserving the existing unknown-column
and unmodeled-type validation for unique fields.
In `@src/backend/services/analytics/src/domain/field_catalog/model.rs`:
- Around line 88-104: The catalog currently carries raw identifier strings
beyond the load boundary. In
src/backend/services/analytics/src/domain/field_catalog/model.rs lines 88-104,
change Dataset and Field to store the appropriate parsed DatasetKey,
RelationName, and FieldName newtypes; in
src/backend/services/analytics/src/domain/field_catalog/loader.rs lines 79-92,
parse each deserialized identifier at catalog construction and reject invalid
values, using the newtypes’ parse APIs so raw Strings do not propagate into
later layers.
---
Nitpick comments:
In `@src/backend/services/analytics/src/domain/field_catalog/loader.rs`:
- Around line 282-338: Consolidate the four rejection tests around build into
one table-driven test, preserving each existing invalid document/snapshot setup
and expected CatalogError match. Define the test return alias as type R =
Result<(), Box<dyn Error>>, iterate through labeled cases, and include each case
label in assertion messages so failures identify the specific rejection
scenario.
In `@src/backend/services/analytics/src/domain/field_catalog/mod.rs`:
- Around line 1-4: Remove narrative comments at
src/backend/services/analytics/src/domain/field_catalog/mod.rs:1-4 and :13-14,
src/backend/services/analytics/src/main.rs:121-121 and :147-148, and
src/backend/services/analytics/src/domain/field_catalog/live_tests.rs:1-9;
replace the documentation comment at
src/backend/services/analytics/src/main.rs:93-95 with the equivalent Clap help
attribute, preserving the existing CLI help text. The main.rs:121 comment should
be removed because the branch invokes render directly, and move live-test
invocation guidance to design documentation if it must remain.
In `@src/backend/services/analytics/src/domain/field_catalog/model.rs`:
- Around line 3-4: Remove the service-local documentation comments from
src/backend/services/analytics/src/domain/field_catalog/model.rs lines 3-4 and
the adjacent model types, without changing code behavior. Remove the module
header from src/backend/services/analytics/src/domain/field_catalog/loader.rs
lines 1-7, retaining durable context only in the design document. At lines
216-217, remove the explanatory test comment and rename the test and strengthen
its assertions so they clearly express the validation rule.
In `@src/backend/services/analytics/src/domain/mod.rs`:
- Line 5: Change the field_catalog module declaration in mod.rs from public to
crate-visible by using the smallest visibility that supports existing internal
callers. Preserve external visibility only if another crate directly imports
field_catalog.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 205e9dc2-819e-4a59-a083-c1bbf6c1a6c3
📒 Files selected for processing (9)
docs/domain/semantic-layer/specs/DESIGN.mdsrc/backend/services/analytics/src/domain/field_catalog/live_tests.rssrc/backend/services/analytics/src/domain/field_catalog/loader.rssrc/backend/services/analytics/src/domain/field_catalog/mod.rssrc/backend/services/analytics/src/domain/field_catalog/model.rssrc/backend/services/analytics/src/domain/field_catalog/roles.yamlsrc/backend/services/analytics/src/domain/field_catalog/types.snapshot.jsonsrc/backend/services/analytics/src/domain/mod.rssrc/backend/services/analytics/src/main.rs
Comment on lines
+285
to
+287
| **Implementation status (#2208, Phase 1 slice 2):** the build-time catalog is shipped in `domain/field_catalog/` (Rust). Because the dbt `schema.yml` files carry no `meta:` type/role annotations, the source is **hybrid**: column **types** come from a committed ClickHouse type snapshot (`types.snapshot.json`, generated from `system.columns` and kept honest by an ignored drift test against the live warehouse), and **roles** are authored in `roles.yaml`. The generator joins the two and rejects any role that names an absent column or an unmodeled type, so the embedded catalog is a consistent validation universe at build time (offline tests) with no live warehouse needed in CI. Inspect via `analytics field-catalog`. | ||
|
|
||
| Coverage is the source families with a clean, self-contained entity and tenant at silver — **git, ai, collab, wiki** (12 datasets). Deferred to the extraction slice (slice 5), where roles are verified against real measures: **task** (`class_task_worklogs`/`comments` carry no tenant column and key on `author_id`, not an email — needs a tenant join and identity resolution), the reference/join relations with no direct entity (repository/branch, git PR comment/commit/reviewer, task statuses/projects/sprints/field-metadata, wiki engagement), and derived populations (active-day, focus). Two cross-family facts the catalog already absorbs: the tenant column name is **not** uniform (`tenant_id` for git/collab/wiki, `insight_tenant_id` for ai), named per dataset in `roles.yaml`; and `Decimal(p,s)` amount columns are a first-class field type distinct from `Float`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the catalog source and construction model.
This text describes a build-time generator. The implementation constructs the embedded catalog lazily through OnceLock::get_or_init. It also conflicts with Line 291, which still says dbt schema.yml metadata is the source and roles are never hand-maintained. Update this section and the responsibility scope so they describe the same hybrid roles.yaml and type-snapshot model.
🤖 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 `@docs/domain/semantic-layer/specs/DESIGN.md` around lines 285 - 287, Update
the catalog description and responsibility scope to match the implementation’s
lazy OnceLock::get_or_init construction, using roles.yaml for authored roles and
types.snapshot.json for column types. Remove or revise the conflicting claim
that dbt schema.yml metadata is the source and roles are never hand-maintained,
while preserving the documented dataset coverage and deferred responsibilities.
Comment on lines
+46
to
+55
| let rows = client | ||
| .query("SELECT name, type FROM system.columns WHERE database = ? AND table = ?") | ||
| .bind(dataset.database.as_str()) | ||
| .bind(dataset.table.as_str()) | ||
| .fetch_all::<ColumnRow>() | ||
| .await | ||
| .unwrap_or_else(|e| panic!("querying columns of {}: {e}", dataset.relation())); | ||
|
|
||
| let live: BTreeMap<String, String> = | ||
| rows.into_iter().map(|r| (r.name, r.ch_type)).collect(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg 'src/backend/services/analytics/src/domain/field_catalog/live_tests\.rs$|clickhouse|ColumnRow' || true
echo "== target file outline/contents =="
if [ -f src/backend/services/analytics/src/domain/field_catalog/live_tests.rs ]; then
wc -l src/backend/services/analytics/src/domain/field_catalog/live_tests.rs
sed -n '1,140p' src/backend/services/analytics/src/domain/field_catalog/live_tests.rs | nl -ba
fi
echo "== all system.columns query usages =="
rg -n 'system\.columns|fetch_all::<ColumnRow>|fetch_all\(' src/backend/services/analytics || true
echo "== lock/clickhouse deps mentions =="
rg -n 'clickhouse|Client|fetch_all|limit|LIMIT' src/backend/services/analytics/Cargo.toml src/backend/services/analytics -g '*.rs' -g 'Cargo.toml' || trueRepository: constructorfabric/insight
Length of output: 1500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact target file contents with line numbers =="
sed -n '1,220p' src/backend/services/analytics/src/domain/field_catalog/live_tests.rs | nl -ba
echo "== lock files for package version context, if present =="
git ls-files '*lock*' 'Cargo.lock' | sed -n '1,40p'
if [ -f Cargo.lock ]; then
python3 - <<'PY'
import re
p='Cargo.lock'
txt=open(p,errors='ignore').read().split('[[package]]\n')[1:]
for blk in txt:
if 'clickhouse' in blk.lower() or 'click' in blk.lower():
m=re.search(r'name\s*=\s*"([^"]*)"\nversion\s*=\s*"([^"]*)"', blk)
print(m.group(0).replace('\n',' | ') if m else blk[:500])
PY
fiRepository: constructorfabric/insight
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== line count =="
wc -l src/backend/services/analytics/src/domain/field_catalog/live_tests.rs
wc -l src/backend/libs/insight-clickhouse/src/query.rs
echo "== target file contents =="
sed -n '1,130p' src/backend/services/analytics/src/domain/field_catalog/live_tests.rs
echo "== query.rs contents =="
sed -n '1,180p' src/backend/libs/insight-clickhouse/src/query.rs
echo "== all system.columns query usage context =="
rg -n -C 4 'system\.columns|fetch_all::<ColumnRow>|fetch_all\(' src/backend/services/analytics src/backend/libs/insight-clickhouse || trueRepository: constructorfabric/insight
Length of output: 25872
Bound the system.columns response in the live-schema test.
This test calls fetch_all::<ColumnRow>() on an unbounded SELECT name, type FROM system.columns WHERE database = ? AND table = ?, so a relation with too many columns can exhaust test memory. Add a response-size limit in the query/streamed read, or fetch N + 1 rows and report a clear over-limit failure instead of panicking.
🤖 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 `@src/backend/services/analytics/src/domain/field_catalog/live_tests.rs` around
lines 46 - 55, Bound the `system.columns` read in the live-schema test around
the `fetch_all::<ColumnRow>()` call, using the project’s established query limit
or streamed-read mechanism. Ensure relations exceeding the limit produce a clear
over-limit failure while preserving the existing `BTreeMap` construction for
valid responses.
Source: Coding guidelines
Comment on lines
+27
to
+34
| pub fn field_catalog() -> &'static FieldCatalog { | ||
| CATALOG.get_or_init(|| { | ||
| #[expect( | ||
| clippy::expect_used, | ||
| reason = "roles.yaml + types.snapshot.json are embedded at compile time and pinned by the offline consistency tests; a failure here is a build defect, not a runtime condition" | ||
| )] | ||
| build(ROLES_YAML, TYPES_SNAPSHOT_JSON).expect("embedded field catalog must be consistent") | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Return catalog initialization errors instead of panicking.
Line 33 converts YAML or JSON parse failures into a panic at the first caller. A malformed embedded artifact can therefore terminate the calling task instead of producing a typed initialization failure.
Store the initialization Result in OnceLock, return a typed error, or validate the catalog during startup before the service accepts requests.
As per coding guidelines, “Use Result for every fallible operation; panics must never cross a request boundary.”
🤖 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 `@src/backend/services/analytics/src/domain/field_catalog/loader.rs` around
lines 27 - 34, Update field_catalog and the CATALOG initialization around
build(ROLES_YAML, TYPES_SNAPSHOT_JSON) to propagate a typed initialization
Result instead of calling expect or panicking. Store the Result in OnceLock or
otherwise return the fallible outcome to callers, and update affected call sites
to handle the initialization error before serving requests.
Source: Coding guidelines
Comment on lines
+125
to
+146
| let mut fields = Vec::with_capacity(roles.fields.len()); | ||
| for entry in &roles.fields { | ||
| let ch_type = columns | ||
| .get(&entry.name) | ||
| .ok_or_else(|| CatalogError::UnknownColumn { | ||
| dataset: roles.key.clone(), | ||
| relation: relation.clone(), | ||
| field: entry.name.clone(), | ||
| })?; | ||
| let (ty, nullable) = | ||
| FieldType::normalize(ch_type).ok_or_else(|| CatalogError::UnmodeledType { | ||
| dataset: roles.key.clone(), | ||
| field: entry.name.clone(), | ||
| ch_type: ch_type.clone(), | ||
| })?; | ||
| fields.push(Field { | ||
| name: entry.name.clone(), | ||
| role: entry.role, | ||
| ty, | ||
| nullable, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject duplicate field names within a dataset.
seen only rejects duplicate dataset keys. If roles.yaml repeats a dimension or value field, this loop adds both entries and assert_role_invariants still returns Ok. The catalog then contains two definitions for one physical column.
Track field names per dataset and return a typed DuplicateField error.
Proposed fix
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, BTreeSet};
pub enum CatalogError {
+ #[error("dataset {dataset}: duplicate field {field}")]
+ DuplicateField { dataset: String, field: String },
}
- let mut fields = Vec::with_capacity(roles.fields.len());
+ let mut fields = Vec::with_capacity(roles.fields.len());
+ let mut seen_fields = BTreeSet::new();
for entry in &roles.fields {
+ if !seen_fields.insert(entry.name.as_str()) {
+ return Err(CatalogError::DuplicateField {
+ dataset: roles.key.clone(),
+ field: entry.name.clone(),
+ });
+ }
let ch_type = columns📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let mut fields = Vec::with_capacity(roles.fields.len()); | |
| for entry in &roles.fields { | |
| let ch_type = columns | |
| .get(&entry.name) | |
| .ok_or_else(|| CatalogError::UnknownColumn { | |
| dataset: roles.key.clone(), | |
| relation: relation.clone(), | |
| field: entry.name.clone(), | |
| })?; | |
| let (ty, nullable) = | |
| FieldType::normalize(ch_type).ok_or_else(|| CatalogError::UnmodeledType { | |
| dataset: roles.key.clone(), | |
| field: entry.name.clone(), | |
| ch_type: ch_type.clone(), | |
| })?; | |
| fields.push(Field { | |
| name: entry.name.clone(), | |
| role: entry.role, | |
| ty, | |
| nullable, | |
| }); | |
| } | |
| let mut fields = Vec::with_capacity(roles.fields.len()); | |
| let mut seen_fields = BTreeSet::new(); | |
| for entry in &roles.fields { | |
| if !seen_fields.insert(entry.name.as_str()) { | |
| return Err(CatalogError::DuplicateField { | |
| dataset: roles.key.clone(), | |
| field: entry.name.clone(), | |
| }); | |
| } | |
| let ch_type = columns | |
| .get(&entry.name) | |
| .ok_or_else(|| CatalogError::UnknownColumn { | |
| dataset: roles.key.clone(), | |
| relation: relation.clone(), | |
| field: entry.name.clone(), | |
| })?; | |
| let (ty, nullable) = | |
| FieldType::normalize(ch_type).ok_or_else(|| CatalogError::UnmodeledType { | |
| dataset: roles.key.clone(), | |
| field: entry.name.clone(), | |
| ch_type: ch_type.clone(), | |
| })?; | |
| fields.push(Field { | |
| name: entry.name.clone(), | |
| role: entry.role, | |
| ty, | |
| nullable, | |
| }); | |
| } |
🤖 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 `@src/backend/services/analytics/src/domain/field_catalog/loader.rs` around
lines 125 - 146, Update the field-loading logic around the fields collection to
track names already encountered within each dataset, and reject repeats before
adding a Field. Return the typed CatalogError::DuplicateField for duplicate
dimension or value names, while preserving the existing unknown-column and
unmodeled-type validation for unique fields.
Comment on lines
+88
to
+104
| pub struct Field { | ||
| pub name: String, | ||
| pub role: FieldRole, | ||
| pub ty: FieldType, | ||
| pub nullable: bool, | ||
| } | ||
|
|
||
| /// A queryable relation with its exposed, typed, role-annotated fields — the | ||
| /// unit a measure aggregates and the validation universe an expression is | ||
| /// checked against. | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub struct Dataset { | ||
| pub key: String, | ||
| pub database: String, | ||
| pub table: String, | ||
| pub read_discipline: ReadDiscipline, | ||
| pub fields: Vec<Field>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Parse catalog identifiers at the load boundary.
The loader deserializes key, database, table, and field name as raw String values. The model then stores those raw values and constructs relations with format!. This makes later layers re-handle unparsed identifiers.
src/backend/services/analytics/src/domain/field_catalog/model.rs#L88-L104: store parsedDatasetKey,RelationName, andFieldNamevalues instead of raw strings.src/backend/services/analytics/src/domain/field_catalog/loader.rs#L79-L92: parse and reject invalid identifiers when deserializing or building the catalog.
As per coding guidelines, “Parse, don't validate: introduce boundary newtypes such as RelationName::parse(&str) -> Option<RelationName> and do not carry raw String values through layers.”
📍 Affects 2 files
src/backend/services/analytics/src/domain/field_catalog/model.rs#L88-L104(this comment)src/backend/services/analytics/src/domain/field_catalog/loader.rs#L79-L92
🤖 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 `@src/backend/services/analytics/src/domain/field_catalog/model.rs` around
lines 88 - 104, The catalog currently carries raw identifier strings beyond the
load boundary. In
src/backend/services/analytics/src/domain/field_catalog/model.rs lines 88-104,
change Dataset and Field to store the appropriate parsed DatasetKey,
RelationName, and FieldName newtypes; in
src/backend/services/analytics/src/domain/field_catalog/loader.rs lines 79-92,
parse each deserialized identifier at catalog construction and reject invalid
values, using the newtypes’ parse APIs so raw Strings do not propagate into
later layers.
Source: Coding guidelines
This was referenced Aug 5, 2026
Contributor
Author
|
Dropping this PR. It reinvented a schema-snapshot the repo already produces and gates ( Salvageable from this branch for the redesign: the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Phase 1, slice 2 of the Semantic Layer definition core (ADR-001): the field catalog — the typed, role-annotated view of each dataset that is the compiler's validation universe. This is what makes capability a projection of definitions rather than of stored data.
The source problem, and the chosen fix
IMPLEMENTATION assumed the catalog is generated from dbt
schema.ymlmeta:blocks — but those don't exist (nometa:, nodata_type, no role annotations anywhere in the dbt schemas). So the source is hybrid (the approach chosen for this slice):system.columnstypes.snapshot.json(drift-gated against live CH)roles.yaml(tenant / entity / event_time / dimension / value / subject)domain/field_catalog/(Rust) joins the two into a typedFieldCatalogand rejects any role that names a column the warehouse doesn't have or a ClickHouse type the layer can't model. So the embedded catalog is a consistent validation universe at build time, with no live warehouse in CI.Why a committed snapshot + drift test
Build-time validation (the "definitions validate against the catalog" gate) must work offline in CI. So types are captured into a committed snapshot; an ignored drift test (
INTEGRATION_TESTS_CLICKHOUSE_URL=…) re-checks every catalogued field's type against live ClickHouse, so schema drift is caught and the snapshot regenerated. Same golden-snapshot + drift-gate idiom asopenapi.json.Scope
Seed datasets are the two git relations with a direct entity:
class_git_commits,class_git_pull_requests. Relations without a self-contained entity (e.g.class_git_file_changes, which needs a join to commits) are derived-dataset cases catalogued during extraction (slice 5). Inspect withanalytics field-catalog.Test
Nullable/LowCardinality/DateTime64), and every rejection path (absent column, unmodeled type, missing entity, absent relation).system.columnsfor every catalogued field.cargo clippy/cargo fmt --checkclean; full offline suite 570 green.Extends the semantic-layer DESIGN field-catalog component with slice-2 status, recording the hybrid-source deviation from dbt
meta:blocks.Part of #2208
Part of #1803
Summary by CodeRabbit