Fixes 33531: keep dedicated-mapper fields in the RDF indexing field set - #33533
Conversation
RdfIndexingFields picked the fields to load by reusing RdfPropertyMapper.isIgnoredEntityField, but IGNORED_PROPERTIES mixes two reasons for skipping a field: audit data with no place in the graph (changeDescription, votes, testCaseResult, identityProviderSubject), and fields a dedicated structured-emission step owns and that must therefore not also be written as opaque JSON literals (tableConstraints -> emitTableConstraints, profile -> RdfQualityMapper, pipelineStatus -> RdfActivityMapper, usageSummary -> RdfUsageMapper). The second group still has to be fetched. Reusing the predicate dropped all four before their mappers ever ran, so table constraints, profiles, pipeline status and usage never reached the graph. RdfIndexingFieldsTest.retainsInputsOfDedicatedRdfMappers fails on main today: forSupportedFields returns [] for those four fields. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
…tion
Review feedback: the DEDICATED_MAPPER_FIELDS set added to RdfIndexingFields
duplicated a subset of RdfPropertyMapper.IGNORED_PROPERTIES, and the two would
have to be kept in sync by hand - a new dedicated-mapper field added to
IGNORED_PROPERTIES alone would silently reintroduce this bug.
Remove the duplication instead. IGNORED_PROPERTIES was always two sets wearing
one name - its own comment said so - so split it along that seam:
NON_GRAPH_PROPERTIES audit data, inline time-series and auth material
that must never be fetched or emitted
DEDICATED_MAPPER_PROPERTIES fields that DO belong in the graph but are
emitted by emitStructuredProperties, so the
generic loop must not also write them as JSON
IGNORED_PROPERTIES is now the union, so isIgnoredEntityField is unchanged, and
the new isIndexableEntityField answers the different question RdfIndexingFields
actually asks: must this field be loaded before translation? There is no second
set to drift.
Also adds two tests pinning the emitter contract that motivates all of this:
with tableConstraints present, emitStructuredProperties produces om:hasConstraint
relations; with it absent it produces nothing at all, silently. Those pass on
main too - they characterise the starvation. The regression test for the bug
itself remains RdfIndexingFieldsTest.retainsInputsOfDedicatedRdfMappers, which
fails on main with `expected: <[pipelineStatus, profile, tableConstraints,
usageSummary]> but was: <[]>`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Good catch — addressed in a939977, and it's worth answering the second question too, because it's the whole point of the change. We do create the relations — they were being starved
RdfJsonNode.array(entityJson, "tableConstraints")
.ifPresent(c -> emitTableConstraints(c, entity.getFullyQualifiedName(), entityResource, model));
RdfJsonNode.object(entityJson, "profile")
.ifPresent(p -> RdfQualityMapper.emitTableProfile(p, entityResource, model));
RdfJsonNode.object(entityJson, "pipelineStatus")
.ifPresent(s -> RdfActivityMapper.emitPipelineActivity(s, entity.getFullyQualifiedName(), entityResource, baseUri, model));
RdfJsonNode.object(entityJson, "usageSummary")
.ifPresent(u -> RdfUsageMapper.emitUsageSummary(u, entityResource, model));
The bug is one layer upstream, in what gets fetched. static Optional<JsonNode> field(JsonNode parent, String fieldName) {
return object(parent).map(value -> value.get(fieldName)).filter(value -> !nullOrEmpty(value));
}
So: the relations are built correctly; the data never reached the builder. On the duplication — you're right, and it's now goneRather than exposing a second constant to keep in sync, I removed the second set entirely. // never fetched, never emitted: audit data, inline time-series, auth material
private static final Set<String> NON_GRAPH_PROPERTIES =
Set.of("changeDescription", "votes", "testCaseResult", "identityProviderSubject");
// fetched and emitted, but by emitStructuredProperties rather than the generic loop
private static final Set<String> DEDICATED_MAPPER_PROPERTIES =
Set.of("tableConstraints", "profile", "pipelineStatus", "usageSummary");
private static final Set<String> IGNORED_PROPERTIES =
Stream.concat(NON_GRAPH_PROPERTIES.stream(), DEDICATED_MAPPER_PROPERTIES.stream())
.collect(Collectors.toUnmodifiableSet());
return supportedFields.stream().filter(RdfPropertyMapper::isIndexableEntityField).sorted().toList();There is no set to keep in sync: add a field to TestsTwo tests added to The regression test for the bug is still Full run after the change: 53 tests green across |
Code Review ✅ Approved 1 closed / 1 findingsSplits ✅ 1 closed✅ Quality: DEDICATED_MAPPER_FIELDS duplicates a subset of IGNORED_PROPERTIES
OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
✅ Playwright Results — workflow succeededValidated commit ✅ 4507 passed · ❌ 0 failed · 🟡 8 flaky · ⏭️ 1 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 54m 16s ⏱️ Max setup 5m 9s · max shard execution 23m 10s · max shard-job elapsed before upload 26m 31s · reporting 16s 🌐 219.21 requests/attempt · 2.23 app boots/UI scenario · 41.37% common-shard skew Optimization targets still in progress:
🟡 8 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
1 participant
Describe your changes:
Fixes #33531
Salvaged from the closed PR #33248 (tracking: #33528).
RdfIndexingFields.forSupportedFieldschose which fields to load for RDF indexing by reusingRdfPropertyMapper.isIgnoredEntityField. ButIGNORED_PROPERTIESmixes two different reasons forskipping a field — and its own comment says so:
changeDescription,votes,testCaseResult,identityProviderSubject;JSON literals":
tableConstraints(emitTableConstraints),profile(RdfQualityMapperDQVmeasurements),
pipelineStatus(RdfActivityMapperprov:Activity),usageSummary(
RdfUsageMapperusage counts).Category 2 still has to be fetched. Reusing the predicate dropped all four from the entity before
their mappers ever ran, so table constraints, table profiles, pipeline status and usage summaries
never reached the knowledge graph at all.
This keeps an explicit
DEDICATED_MAPPER_FIELDSset inRdfIndexingFieldsand retains those four,leaving category 1 filtered out exactly as before.
Type of change:
High-level design:
N/A — small bug fix. The two categories now have two predicates instead of one overloaded one.
Tests:
Use cases covered
Tablecarries its constraints and profile into RDF; aPipelinecarries itspipelineStatus; usage summaries reachRdfUsageMapper.changeDescription,votesandtestCaseResultare still excluded.Unit tests
RdfIndexingFieldsTest.retainsInputsOfDedicatedRdfMappers, which fails onmain— todayforSupportedFieldsreturns[]for those four fields:After the fix:
The pre-existing test in the same class (which asserts the category-1 fields stay filtered) is
unchanged and still passes.
Backend integration tests
Not applicable — no API change.
docs/rdf-ontology-contract.mddescribes the predicate contractthese mappers emit.
Ingestion integration tests
Not applicable.
Playwright (UI) tests
Not applicable — no UI change.
Manual testing performed
mvn -pl openmetadata-service test -Dtest='RdfIndexingFieldsTest,RdfPropertyMapperTest'— 38 tests, BUILD SUCCESS.retainsInputsOfDedicatedRdfMappersfails with the output above.mvn spotless:apply -pl openmetadata-service— 3032 files clean, 0 changed.Note for reviewers: this changes what is fetched for RDF indexing, so existing graphs need a
re-index to pick up the previously-missing triples.
UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #33531.🤖 Generated with Claude Code
The PR appears safe to merge because the revised predicate restores required mapper inputs without exposing the fields intentionally excluded from RDF.
Summary
This PR separates RDF field-loading eligibility from generic property-emission eligibility.
tableConstraints,profile,pipelineStatus, andusageSummarywhile loading entities for RDF indexing.Reviews (2) · Last reviewed commit: "Make RdfPropertyMapper the single source..."