[BUG] Divisions: allow both is_land and is_territorial to be true#546
Merged
Dana Bauer (danabauer) merged 11 commits intoJul 8, 2026
Merged
Conversation
Replace @radio_group (exactly one must be true) with a new @require_any_true constraint (at least one must be true) on DivisionArea and DivisionBoundary models. Changes: - Add require_any_true constraint to overture-schema-system - Update division_area.py and division_boundary.py to use it - Change division_boundary.yaml schema from oneOf to anyOf - Remove bad-not-both counterexample (now valid) - Add both_land_territorial positive examples - Update codegen model_constraints.py for new constraint type - Regenerate baseline schemas Closes #542 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
🗺️ Schema reference docs preview is live!
Note ♻️ This preview updates automatically with each push to this PR. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Copilot started reviewing on behalf of
Alex Iannicelli (atiannicelli)
June 18, 2026 21:05
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the divisions schema to allow is_land and is_territorial to both be true by replacing the “exactly one true” constraint (@radio_group / oneOf) with an “at least one true” constraint (@require_any_true / anyOf) across the Pydantic models and the hand-written YAML schema.
Changes:
- Add a new
@require_any_truemodel constraint inoverture-schema-systemand export it. - Update
DivisionArea/DivisionBoundaryto use@require_any_true("is_land", "is_territorial"). - Update the YAML + baseline JSON schemas and examples/counterexamples to reflect
anyOfbehavior and allow both flags to betrue.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| schema/divisions/division_boundary.yaml | Switches oneOf → anyOf for land/territorial flags in the YAML schema. |
| packages/overture-schema-system/src/overture/schema/system/model_constraint/require_any_true.py | Introduces the new model constraint and JSON Schema generation hook. |
| packages/overture-schema-system/src/overture/schema/system/model_constraint/init.py | Exports require_any_true / RequireAnyTrueConstraint. |
| packages/overture-schema-divisions-theme/src/overture/schema/divisions/division_boundary.py | Replaces @radio_group with @require_any_true. |
| packages/overture-schema-divisions-theme/src/overture/schema/divisions/division_area.py | Replaces @radio_group with @require_any_true. |
| packages/overture-schema-codegen/src/overture/schema/codegen/extraction/model_constraints.py | Adds description + field-affecting logic for the new constraint type. |
| packages/overture-schema-divisions-theme/tests/division_boundary_baseline_schema.json | Updates generated baseline schema to use anyOf. |
| packages/overture-schema-divisions-theme/tests/division_area_baseline_schema.json | Updates generated baseline schema to use anyOf. |
| examples/divisions/division_boundary/both_land_territorial.yaml | Makes the “both true” example valid (removes expected-error, adjusts fields). |
| examples/divisions/division_area/both_land_territorial.yaml | Adds a new valid “both true” example for division areas. |
| counterexamples/divisions/division_boundary/bad-not-both.yaml | Removes a counterexample that is now valid under anyOf. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
becab7a to
3df9e21
Compare
Victor Schappert (vcschapp)
left a comment
Collaborator
There was a problem hiding this comment.
Two comments:
- It might be useful to generalize
require_any_true. - The Python code doesn't use
examples/andcounterexamples/it usesreference/examples/andreference/counterexamples/- apologies for this inconsistency but could these be updated also?
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
108710d to
3b64d0a
Compare
John McCall (lowlydba)
approved these changes
Jul 2, 2026
TristanDiet-TomTom
approved these changes
Jul 8, 2026
Seth Fitzsimmons (sethfitz)
approved these changes
Jul 8, 2026
Stephen Epps (stepps00)
approved these changes
Jul 8, 2026
Seth Fitzsimmons (sethfitz)
added a commit
that referenced
this pull request
Jul 8, 2026
Generate a PySpark validator and conformance tests for the `require_any_true` model constraint (#546), so divisions' `is_land` / `is_territorial` rule is enforced in Spark, not just Pydantic and Markdown. Support is positive-boolean-`FieldEqCondition`-only, enforced at `RequireAnyTrue.__post_init__` (and independently in base-row synthesis, which reads the raw constraint). The runtime coalesces a null condition to False, which matches Python's `None == value` only for a positive equality; a negated or non-boolean condition would emit a wrong Spark check, so it fails loudly at codegen instead. Pipeline stages: - constraint_dispatch: a self-validating `RequireAnyTrue` descriptor. The `FieldEq` unwrap helpers (`parse_field_eq` / `require_field_eq`) live here, the lowest layer that needs them. - runtime `check_require_any_true`: a disjunction over the conditions with each null coalesced to False. - renderer: lower each condition through the `require_if` path; boolean values render as `F.lit(True)` so ruff's E712 leaves the Column comparison intact -- a latent gap that also affected boolean `require_if` / `forbid_if` conditions. - base_row: satisfy one condition so the valid baseline passes Pydantic. - test renderer: an "all conditions false" mutation. Regenerate the PySpark expressions and conformance tests. The diff spans every theme because the rebase onto post-#546 main also picked up the provider/resource/version string-typing change, which adds min_length and snake_case checks to the shared Sources model. DivisionArea's e2e test moves from radio_group to require_any_true to match the changed source. Signed-off-by: Seth Fitzsimmons <seth@mojodna.net>
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
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.
Summary
Replaces the
@radio_groupconstraint (exactly one must be true) with a new@require_any_trueconstraint (at least one must be true) onDivisionAreaandDivisionBoundarymodels. This allows bothis_landandis_territorialto betruesimultaneously.Changes
@require_any_trueconstraint inoverture-schema-system— validates that at least one field in a group ofboolfields isTrue, generatinganyOfin JSON Schema (vsoneOffrom@radio_group)DivisionAreaandDivisionBoundaryto use@require_any_true("is_land", "is_territorial")division_boundary.yaml) fromoneOf→anyOfbad-not-bothcounterexamples (now valid) and addedboth_land_territorialpositive examplesmodel_constraints.pyto handle the new constraint typeCloses #542