Repository navigation
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain, and the schema, validation, model, and regression-test changes collectively address duplicate-entry and uniqueness handling.
No open issues 🎉
What changed in this PR
This pull request prevents duplicate entries in core-spec keys and ontology lists, adds semantic validation for case-insensitive and reordered key duplicates, and keeps the Python models aligned with the updated schemas.
Changes:
- Adds
uniqueItemsconstraints to core-spec and ontology schemas. - Validates repeated key fields and redundant unique-key definitions.
- Adds Pydantic validators mirroring schema uniqueness rules.
- Expands regression coverage for schema, model, key, and ontology validation.
This pull request is too complex or risky for automated approval.
| File | Description |
|---|---|
validation/validate.py |
Adds case-insensitive key-declaration checks and redundancy warnings. |
validation/tests/test_validate.py |
Tests duplicate, case-variant, undeclared, and redundant key scenarios. |
validation/tests/test_ontology.py |
Tests duplicate detection across ontology lists. |
python/tests/test_models.py |
Verifies model/schema uniqueness parity, including nested unique keys. |
python/src/ossie/models.py |
Adds Pydantic validators mirroring schema uniqueness constraints. |
ontology/ontology.json |
Rejects repeated entries across ontology expression and naming lists. |
core-spec/ossie-schema.json |
Rejects repeated primary-key fields and duplicate unique-key definitions. |
Review effort: Lite
kayemkim
left a comment
kayemkim
left a comment
There was a problem hiding this comment.
On the tree merged with current main: 37 checks pass on the four Python versions, and validate.py agrees with main on the 78 model and ontology documents I could find, converter fixtures included, so nothing shipped today repeats an entry. The folded cases behave as described: [order_id, ORDER_ID] is an error, the reordered unique_keys pair is a warning with exit 0, and a unique key restating the primary key stays silent. It also merges cleanly over #697.
Not blocking: this is a tightening, so a document with a repeated key entry that validated yesterday fails at the schema step today, which is a line for the migration notes #686 is collecting, and spec.md's description of primary_key could say the fields are distinct.
| "uniqueItems": true | ||
| }, | ||
| "minItems": 1, | ||
| "uniqueItems": true, |
There was a problem hiding this comment.
This outer uniqueItems makes unique_keys: [[email], [email]] a schema error. The validator stops on schema errors ("Semantic checks skipped until schema errors are fixed."), so the author also loses every reference and SQL diagnostic until it's fixed.
Meanwhile [[email], [EMAIL]] and [[id, email], [email, id]] are the same redundancy, and they only get the warning from validate_key_declarations, so those document pass. That function docstring, and the comment on OssieDataset.unique_keys, both call a repeated definition redundant rather than wrong. That points to a warning in every case.
Could we drop this outer uniqueKeys, and the outer AfterValidator(_distinct) on unique_keys in models.py, and let validate_key_declarations warn on exact repeats too?
If so, the repeat check there should compare definitions by position rather than with earlier is not key. Otherwise a YAML alias (unique_keys: [&k [email], *k]) passes the same list twice and isn't reported. The inner uniqueItems on each key and the one on primary_key are fine as errors: a key that names a field twice really is wrong.
| "type": "string" | ||
| }, | ||
| "minItems": 1, | ||
| "uniqueItems": true, |
There was a problem hiding this comment.
These new constraints should also go into core-spec/spec.md. The dataset table rows for primary_key and unique_keys don't say a field can't be named twice, and Version History has no entry. Documents that passed before this PR now fail, so the spec should say why.
For example, "Dataset fields that uniquely identify rows, each named once" for primary_key, the same rule for each unique_keys definition, and a Version History line for the change.
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "uniqueItems": true, |
There was a problem hiding this comment.
Same for the ontology. Since #673, ontology.md states each schema constraint in its field tables ("each must be non-empty"). The rows for extends, derived_by, identify_by, requires and verbalizes don't yet say that entries must be distinct.
Could we add that to those rows (e.g. "each must be non-empty and listed once") and add a Version History entry? Otherwise we bring back the schema/spec drift that #673 just fixed.
No schema used uniqueItems, so a key could name one field twice and a list
could carry the same entry twice:
primary_key: [order_id, order_id]
unique_keys: [[order_id, order_id]]
verbalizes: ["{Order} has {Total}", "{Order} has {Total}"]
A key that repeats a field declares fewer columns than it appears to, and a
repeated entry constrains, derives, names or verbalizes nothing the first
already did.
ossie-schema.json marks primary_key and each definition inside unique_keys.
ontology.json marks the eight lists of names and expressions: extends,
identify_by and verbalizes, and requires and derived_by wherever they appear.
models.py mirrors the two core-spec keys. Pydantic 2 dropped unique_items, so
an AfterValidator stands in for it, as min_length already stands in for
minItems.
The list of unique_keys definitions carries no uniqueItems. A definition
written twice is redundant rather than wrong, and a schema error stops the
semantic checks, so the author would lose every reference and SQL diagnostic
over a document that states nothing untrue.
uniqueItems also compares entries as written, while a key means the set of
fields its entries name, and regular identifiers are case insensitive
(expression_language.md), so a schema keyword could not reach these at all:
primary_key: [order_id, ORDER_ID]
unique_keys: [[order_id, line_number], [line_number, order_id]]
validate_key_declarations reports both, and the verbatim repeat with them. A
key that names one field twice claims a column it does not have, which is an
error. Two unique_keys definitions holding the same set of fields are one key
written twice, which is redundant, so that is a warning. Definitions are
compared by position rather than by identity, because a YAML alias,
unique_keys: [&k [email], *k], makes one list object the value of two.
An entry whose folded form names no declared field is left out of both: it
names nothing to compare, validate_references reports it as undeclared, and
calling it a repeat would name a field the dataset does not have. An empty
entry, which passes the schema because key items carry no minLength, is
dropped the same way but without discarding the entries beside it.
A dataset that declares two field names differing only in case declares one
field twice, so a key naming both spellings names that field twice and is
reported. validate_unique_names compares field names as written and does not
report the declaration itself; closing that gap is a change of its own, and it
does not make the key's two entries two fields.
A unique key that restates the primary key stays silent: DDL commonly declares
both, and examples/tpcds_semantic_model.yaml does so in five of its datasets.
Relationship from_columns and to_columns are left alone as well: they
correspond positionally, so one column of the from dataset may legitimately be
compared with two columns of the to dataset.
The field tables state the constraints alongside the others they already
state. spec.md describes primary_key and unique_keys as naming each field
once, with a paragraph on the recased and reordered cases; ontology.md marks
its seven documented lists in the convention apache#673 introduced and documents the
root requires, which no row covered. Both Version Histories record the change,
and docs/spec_changes.md records its migration impact.
556201c to
f4cd4be
Compare
Summary
No schema used uniqueItems, so a key could name one field twice and a list could carry the same entry twice:
A key that repeats a field declares fewer columns than it appears to, and a repeated entry constrains, derives, names or verbalizes nothing the first already did.
ossie-schema.json marks primary_key, unique_keys and each definition inside unique_keys. ontology.json marks the eight lists of names and expressions: extends, identify_by and verbalizes, and requires and derived_by wherever they appear.
models.py mirrors the two core-spec keys. Pydantic 2 dropped unique_items, so an AfterValidator stands in for it, as min_length already stands in for minItems.
uniqueItems compares entries as written, while a key means the set of fields its entries name, and regular identifiers are case insensitive (expression_language.md), so these still reached the semantic checks:
primary_key: [order_id, ORDER_ID]
unique_keys: [[order_id, line_number], [line_number, order_id]]
validate_key_declarations reports them. A key that names one field twice claims a column it does not have, which is an error. Two unique_keys definitions holding the same set of fields are one key written twice, which is redundant rather than wrong, so that is a warning.
An entry whose folded form names no declared field is left out of both: it names nothing to compare, validate_references reports it as undeclared, and calling it a repeat would name a field the dataset does not have. An empty entry, which passes the schema because key items carry no minLength, is dropped the same way but without discarding the entries beside it.
A dataset that declares two field names differing only in case declares one field twice, so a key naming both spellings names that field twice and is reported. validate_unique_names compares field names as written and does not report the declaration itself; closing that gap is a change of its own, and it does not make the key's two entries two fields.
A unique key that restates the primary key stays silent: DDL commonly declares both, and examples/tpcds_semantic_model.yaml does so in five of its datasets. Relationship from_columns and to_columns are left alone as well: they correspond positionally, so one column of the from dataset may legitimately be compared with two columns of the to dataset.
Related Issues
Checklist
Specification
core-spec/and follow the existing structureOntology
ontology/are consistent with spec changesConverters
Validation
validation/are updated if the spec changedDocumentation
docs/is updated to reflect any user-facing changesCONTRIBUTING.mdis updated if the contribution process changedExamples
examples/are added or updated for any new spec constructsTests
pytest/ CI green)Compliance
NOTICEand/orLICENSEhave been updated per ASF policy