Skip to content

docs(spec-changes): correct the 0.3.0.dev0 history and fix the change audit - #711

Open
jbonofre wants to merge 2 commits into
apache:mainfrom
jbonofre:spec-changes-followup
Open

jbonofre wants to merge 2 commits into
apache:mainfrom
jbonofre:spec-changes-followup

Conversation

@jbonofre

Copy link
Copy Markdown
Member

Summary

Follow-up to #686.

History page (docs/spec_changes.md)

Audit (scripts/audit-spec-changes.py)

  • Drop --first-parent. On a branch that had merged main, it followed the branch and skipped the main commits merged after the branch forked: on docs: document significant specification changes and migration impact #686's head it found 44 candidate commits against 53 on main. Without it, both gave 53, so main itself gains no extra candidates.
  • Resolve the default history file and the pathspecs from the repository root. Run from a subdirectory, the audit reported no candidates at all.
  • Read --since as midnight UTC, matching the dates in the history. A bare date meant the current local time on that day, so earlier merges that day were dropped.

Tests and CI

  • Regression tests for the three fixes. They fail against the previous script.
  • Strip GIT_* variables and the user's git config from the test environment, so the tests cannot act on the developer's repository when run from a git hook.
  • Run scripts/tests/ in Validation CI, and trigger the workflow on changes to the audit script or its tests.

Update the apache#577 and apache#591 rows, which later PRs superseded: apache#587 set the
default NULL placement in window ORDER BY, and apache#633 lets dataset-scoped
metrics reference source columns as source.column. State the apache#617
precedence of expression.sql as the MUST the spec makes it.

Add the breaking changes merged in this window (apache#397 root dialects and
vendors removed, apache#400 ontology uri renamed to iri, apache#486 name resolution),
together with apache#587, apache#633 and the Layer 3 interface (apache#653).
Drop --first-parent: on a branch that has merged main, it followed the
branch and skipped every main commit merged after the branch forked.
Resolve the default history file and the pathspecs from the repository
root, so running from a subdirectory no longer reports no candidates.
Read --since as midnight UTC, matching the dates in the history, instead
of the current local time on that date.

Strip GIT_* variables and the user's git config from the test
environment, so the tests cannot act on the developer's repository when
run from a git hook, and run them in Validation CI.

@christianeu-db christianeu-db left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Left two notes; feel free to address them as follow-ups.

Comment thread docs/spec_changes.md
| 2026-10-22 | Core [#591](https://github.com/apache/ossie/pull/591) | **Dataset-scoped metrics.** Metrics can live with the dataset they describe instead of all metrics living at the model root. | Additive optional `datasets[].metrics`. A dataset metric references its own dataset's declared fields by unqualified name; model-scoped metric expressions use qualified `dataset.field` names. The later [#633](https://github.com/apache/ossie/pull/633) also lets dataset metrics reference source columns as `source.column`. Consumers must account for both scopes when reading metrics. |
| 2026-10-22 | Core [#486](https://github.com/apache/ossie/pull/486) | **Name resolution for fields, keys, relationships, and metrics.** The specification did not say whether these names meant dataset fields or source columns, and converters disagreed. Following the dev@ discussion, it now mirrors SQL views: field expressions read source columns, and the constructs above a dataset read its fields. | **Breaking.** `primary_key`, `unique_keys`, relationship `from_columns`/`to_columns`, and metric expressions refer to dataset fields, not source columns, and model-scoped metric expressions must use qualified `dataset.field` references. A dataset that declares no fields exposes nothing to keys, relationships, or model-scoped metrics. Converters that read these names as warehouse columns must resolve them as fields; SQL generators replace each field reference with the field's expression. See [name resolution](../core-spec/spec.md#name-resolution). |
| 2026-10-26 | Core [#617](https://github.com/apache/ossie/pull/617) | **Portable `expression.sql` and open vendor dialect tokens.** The Ossie-defined portable expression becomes primary, while vendor-specific dialects stay opaque and extensible. | Existing `expression.dialects` remains accepted; writers should prefer `expression.sql` for portable SQL, and readers must give it precedence over a duplicate `OSSIE_SQL_2026` dialect entry. Dialect names are no longer a closed enum; consumers must tolerate unknown values. |
| 2026-11-01 | Ontology [#668](https://github.com/apache/ossie/pull/668) | **Non-empty ontology identifiers.** Validation now rejects empty names, IRIs, references, and mapping SQL expressions, matching existing restrictions elsewhere. | Authors should supply non-empty values for identifying strings. Some malformed documents that previously passed schema validation now fail earlier, with schema-layer errors. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#574 and #582 tighten the schema the same way #668 does: primary_key: [] and an object mapping with neither expression nor referent_mappings used to validate and now fail. Should they get rows next to this one, or is leaving them out intended?

history = (args.history or root / "docs/spec_changes.md").read_text(encoding="utf-8")
result = subprocess.run(
["git", "log", "--first-parent", f"--since={args.since}",
["git", "-C", str(root), "log", f"--since={args.since} 00:00:00 +0000",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without --first-parent, the path-filtered git log skips a real Merge pull request #N merge commit:

$ git merge --no-ff -m "Merge pull request #9 from contributor/spec" contributor/spec
$ python scripts/audit-spec-changes.py --since 2026-10-01

Recorded significant PRs: 1; candidate PRs not in history: 0

The documented 2026-09-01 window has only squash merges, but with --since 2025-01-01 the script on main finds nine older merges (such as #54 and #65) that this one misses. Could we add --show-pulls here, which brings them back and still passes the new tests?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @christianeu-db for identifying the ordinary-merge coverage gap.

I independently reproduced it against the current #711 HEAD (7391337) using synthetic Git histories covering ordinary merges, diverged main, synchronized feature branches, and nested merges.

The original implementation missed merge PRs in eight targeted cases.

Applying --show-pulls to the path-filtered git log command, together with the additional regression cases, resulted in 36 passing tests in the Python 3.14 verification job. All four Python 3.11–3.14 Fork verification jobs completed successfully.

Evidence:
https://github.com/zljie/OSI/actions/runs/42954741409

This verifies a temporary Fork candidate. The fix has not been pushed into #711.

Apache upstream CI has passed its existing tests, but those tests do not cover these ordinary-merge cases.

Would you prefer incorporating --show-pulls and the regression tests into #711, or handling them in a separate follow-up?

I have left the thread open for maintainer review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants