[MrBlack1995]

Summary

The spec now gives every expression a primary, portable sql form (OSSIE_SQL_2026), with dialects optional (core-spec/spec.md). The portability grader read only dialects, so a converter that returns an expression in the sql form was scored as if the expression were gone. Even a model returned completely unchanged:

element (input = output, sql form)	before	after
metric total, sql: SUM(sales.amount)	SURVIVED_VENDOR_ONLY, "no expression returned"	PORTABLE (tag preserved)
field sales.amount, sql: amount	SURVIVED_VENDOR_ONLY, "no expression returned"	PORTABLE (tag preserved)

• _dialect_map() reads sql under OSSIE_SQL_2026, so every outcome rule applies to it unchanged.
• PORTABLE is an explicit rule in _classify_present: an authored portable form (the sql form, or a portable dialects entry) returned unchanged under its tag, and no authored portable form returned changed. When an expression carries sql plus an ANSI_SQL copy, a converter that reads and writes only one of them is PORTABLE either way; one that passes a copy through but rewrites the other is REWRITTEN, and the changed form is the one reported. TAG_PRESERVED is set when the OSSIE_SQL_2026 form is among those kept. (Thanks to @jbonofre's review: ordering alone only moved the mis-grade.)
• sql is authoritative: next to it, an OSSIE_SQL_2026 entry in dialects is shadowed and not read, on the input and the returned side (core-spec/spec.md: consumers MUST use sql as the OSSIE_SQL_2026 form).
• sql returned as an OSSIE_SQL_2026 dialect entry, or the other way round, grades PORTABLE: same tag, same text.
• Which tags count as portable is unchanged, and every posted fixture carries one dialect entry, so every existing fixture score stays the same. The one change for other documents: an expression with two portable dialect entries whose second one comes back unchanged is now PORTABLE (it graded by the first entry only). Whether ANSI_SQL should still count as portable now that it is an opaque label belongs with moving the fixtures to the sql form, which I would propose separately.

Validation

• Grader tests: 25 passed at 8102ea9 (13 existing, 12 new: the sql form, sql vs an OSSIE_SQL_2026 entry in both directions, sql winning over that entry, an ANSI_SQL input returned in the sql form grading RELABELED, and for sql plus a dialect entry: only the sql form returned, only an unchanged ANSI_SQL entry returned (equal to sql, and different), both returned, and neither returned unchanged, REWRITTEN; plus two portable entries without sql, the second returned unchanged). Since 3b04057, 27 passed: returning only a shadowed OSSIE_SQL_2026 entry's text is REWRITTEN, a changed sql next to a shadowed entry with the old text is REWRITTEN, a rewritten sql next to an unchanged ANSI_SQL copy is REWRITTEN (these three fail on 8102ea9), and the sql text returned under ANSI_SQL grades RELABELED (fails if the shadowed entry is read).
• The existing identity test on the M0-M3 fixtures still grades every element PORTABLE.
• Standard library only, as before; ruff check clean. Merges cleanly with main; conflicts mechanically with test(compliance): add portability rungs M4-M6 and the FILTER (WHERE) form of M3 #631 and test(compliance): add portability rungs M7 (window ORDER BY) and M8 (ratio-to-total) #700 in grade.py (same functions), so whichever lands second rebases.

Related Issues

Refs apache/ossie-converters#24 (portability measurement). Follows #596.

[jbonofre]

_dialect_map adds the primary sql form after the dialects entries, and _authored_tag picks the first portable key in insertion order.
So when a source has both a primary sql and an ANSI_SQL entry in dialects, the authored tag resolves to ANSI_SQL instead of OSSIE_SQL_2026.

Example source:

 expression:
   sql: "SUM(a)"
   dialects:
     - dialect: ANSI_SQL
       expression: "SUM(a)"

The map is {ANSI_SQL, OSSIE_SQL_2026}, so _authored_tag returns ANSI_SQL. A converter that returns only the primary sql form then gets tag_preserved=False and is graded RELABELED or REWRITTEN instead of PORTABLE. That would misreport a correct conversion.

I would suggest either insert the sql entry first, or make _authored_tag prefer OSSIE_SQL_2026 when it is present.

Could you also add a test in test_grade.py with sql plus a non-Ossie dialects entry, so this ordering is pinned?

[MrBlack1995]

Thanks, reproduced: your example graded RELABELED ("authored ANSI_SQL, returned OSSIE_SQL_2026"). Fixed in the latest commit with your first suggestion: _dialect_map now reads sql first, so _authored_tag resolves to OSSIE_SQL_2026, and an OSSIE_SQL_2026 entry next to sql is skipped since sql is primary. I went with the ordering rather than changing _authored_tag, so models without sql build exactly the same map as before. test_sql_form_is_the_authored_tag_next_to_an_ansi_entry pins it: your example now grades PORTABLE with the tag preserved. 19 grader tests pass.

[MrBlack1995]

@jbonofre whenever you have a moment, this is ready for another look: the fix for your point is in 84c9090 (details in my comment above). It still merges cleanly with main, and the grader tests pass on top of current main (19). Thanks!

[jbonofre]

@MikeNitsenko absolutely! Thanks!

[jbonofre]

Reading sql first makes OSSIE_SQL_2026 the only authored tag whenever both forms are present.
That swaps which case is mis-graded instead of fixing it.

With the spec's own example shape:

expression:
  sql: SUM(a)
  dialects:
    - dialect: ANSI_SQL
      expression: SUM(a)

a converter that only reads and writes dialects returns the ANSI_SQL entry unchanged. _authored_tag gives OSSIE_SQL_2026, which is not in the output, so the RELABELED loop matches ANSI_SQL and reports "authored OSSIE_SQL_2026, returned ANSI_SQL".
If the ANSI copy differs from sql, the unchanged return is graded REWRITTEN. Before this commit it was PORTABLE.

Both forms were authored, so returning either one unchanged should be PORTABLE.

Could we make that an explicit rule in _classify_present instead of relying on insertion order here? Something like:

# PORTABLE: any authored portable (tag, text) pair came back unchanged.
kept = [d for d in in_dialects
        if d in PORTABLE_DIALECTS and out_dialects.get(d) == in_dialects[d]]
if kept:
    preserved = "OSSIE_SQL_2026" in kept
    return Finding(
        input_element.kind, input_element.name, PORTABLE,
        tag_preserved=preserved,
        detail=f"returned as {'OSSIE_SQL_2026' if preserved else kept[0]}",
    )

[MrBlack1995]

You're right, thanks: the ordering only moved the mis-grade. Fixed in 7f82a22 with your rule in _classify_present: any authored portable (tag, text) pair that comes back unchanged is PORTABLE, and tag_preserved is set when the OSSIE_SQL_2026 one is among them.

One small difference from the snippet: the pairs are kept next to the dialect map instead of read from it. The map holds one text per tag, so with sql: SUM(a) plus an OSSIE_SQL_2026 entry SUM(b), the entry was not in it, and a converter returning that entry unchanged would still have missed. That case is tested too.

New tests: your example (only the ANSI_SQL entry returned, equal to sql and different), both forms returned, the OSSIE_SQL_2026 entry next to a different sql, neither returned unchanged (REWRITTEN), and, since the rule is general, two portable entries without sql whose second one comes back unchanged (now PORTABLE; it graded by the first entry only). Four of them fail on the previous commit. 25 grader tests pass, also on top of current main. The posted fixtures each carry one dialect entry, so their scores do not change.

[christianeu-db]

Thanks @MrBlack1995! Left three comments; I think the first one, on the shadowed OSSIE_SQL_2026 entry, should be fixed before merge.

[christianeu-db]

The spec says a top-level sql is authoritative and consumers MUST use it as the OSSIE_SQL_2026 form, but this keeps the shadowed entry, so a returned sql that changed still grades PORTABLE:

# input
expression: {sql: SUM(a)}
# returned: [PORTABLE (tag preserved)] metric total  -- returned as OSSIE_SQL_2026
expression:
  sql: SUM(x)
  dialects: [{dialect: OSSIE_SQL_2026, expression: SUM(a)}]

The input side is the same, and a new test pins it: sql: SUM(a) with an OSSIE_SQL_2026 entry SUM(b), returned as that entry, grades PORTABLE. Could _authored_pairs skip the entry when sql is present, as _dialect_map does?

[MrBlack1995]

Agreed, the spec is clear on this: next to sql the OSSIE_SQL_2026 entry is shadowed. Fixed in 3b04057: the authored forms are now read the way a consumer reads them (_dialect_map, which already skipped the entry), on the input and the returned side, so the separate authored-pairs set was redundant and is gone. Your example is a test now (a changed sql next to a shadowed entry with the old text is REWRITTEN), and the test that pinned the old behaviour is inverted: returning only the shadowed entry's text is a rewrite of the authored form.

[christianeu-db]

Not a blocker: when a converter rewrites sql but passes the ANSI_SQL copy through, this grades PORTABLE and the change to the primary form is not reported:

# input
expression:
  sql: SUM(a)
  dialects: [{dialect: ANSI_SQL, expression: SUM(a)}]
# returned: [PORTABLE] metric total  -- returned as ANSI_SQL
expression:
  sql: SUM(x)
  dialects: [{dialect: ANSI_SQL, expression: SUM(a)}]

Should a returned portable form whose text changed make the element REWRITTEN? With that rule, a converter that returns only one form unchanged still grades PORTABLE, as @jbonofre asked.

[MrBlack1995]

Yes, done in the same commit: an element whose authored portable form comes back changed is REWRITTEN, and the changed form is the one reported, even when another authored form comes back unchanged. A converter that returns only one form unchanged (the other absent) still grades PORTABLE, so the earlier rule holds. Your example is a test.

[christianeu-db]

Not a blocker: this test still passes with the continue in _dialect_map removed, because the unchanged sql pair already grades PORTABLE through kept. A second return of the sql text under ANSI_SQL would pin that sql wins, since it grades RELABELED only while that entry is skipped:

    returned = _doc(metrics=[_sql_metric("total", "SUM(sales.amount)")])
    assert _only(grade.grade(src, returned)).outcome == grade.PORTABLE
    relabeled = _doc(metrics=[_metric("total", "ANSI_SQL", "SUM(sales.amount)")])
    assert _only(grade.grade(src, relabeled)).outcome == grade.RELABELED

[MrBlack1995]

Good catch, added your second return to that test in 3b04057: the sql text under ANSI_SQL grades RELABELED. With the continue in _dialect_map removed it now fails, together with the two new shadowing tests. 27 grader tests pass; the 3 from your examples fail on 8102ea9. Merged with current main the fixtures still validate (the only warnings are the bare model-scoped variants, which #631 moves to dataset-scoped metrics).

[christianeu-db]

LGTM, thanks @MrBlack1995! One note inline for a follow-up. @jbonofre, do you mind taking another look at your change request when you get a chance?

[christianeu-db]

With an ANSI_SQL-only input, an unchanged copy still hides a rewritten sql form, because changed only checks the authored tags:

# input
expression: {dialects: [{dialect: ANSI_SQL, expression: SUM(a)}]}
# returned: [PORTABLE] metric total  -- returned as ANSI_SQL
expression:
  sql: SUM(x)
  dialects: [{dialect: ANSI_SQL, expression: SUM(a)}]

Returned without the copy, sql: SUM(x) grades REWRITTEN. Could changed also count a returned sql that differs from the authored text?

[MrBlack1995]

Thanks, agreed: a returned sql is the primary form, so it should count as changed whenever its text differs from the authored one, whichever tags the input used. I'll take this in the grader follow-up right after this merges (together with the empty-return case from #65: an empty but valid return as DROPPED rather than REJECTED), with your example as a test, so the approval here stays as is.

[jbonofre]

Thanks @MrBlack1995! Reading sql as OSSIE_SQL_2026 closes the bigger gap: on main, an expression that only uses sql has nothing to grade. The remaining mis-grade also happen on main.

I pushed one commit: test_an_unchanged_ansi_entry_that_differs_from_sql_is_portable is now test_dropping_sql_for_a_differing_ansi_entry_is_not_portable. It asserts the intended outcome (returning only an ANSI_SQL entry that differs from the authored sql drops the authoritative form, so it is not PORTABLE) and is marked xfail(strict=True). It also removes two extra blank lines.