[MrBlack1995]

Summary

Follow-up to #596: the next portability rungs, one commit each, on top of the M0-M3 set. Each rung still adds one construct, so a difference in a converter's output localizes to it. Happy to drop or split any of these.

Rung	Construct	Fixture
M3 (new variants)	the same filtered aggregate written with the aggregate FILTER (WHERE ...) clause, which expression_language.md makes REQUIRED, qualified and bare. The existing CASE fixtures now omit ELSE 0 (SUM(CASE WHEN ... THEN amount END)), so both forms are the same aggregate, as the spec's note under FILTER (WHERE ...) states; with ELSE 0 they gave 0 where FILTER gives NULL (found by @idk-arsh's execution check on apache/ossie-converters#24)	m3/m3_filter_clause_qualified.ossie.yaml, m3/m3_filter_clause_bare.ossie.yaml
M4	two datasets, a declared relationship, and a model-scoped metric across the join	m4/m4_cross_dataset.ossie.yaml
M5	metric composition: a model-scoped metric built from two others, carried with their expressions inlined (SUM(sales.amount) / NULLIF(COUNT(sales.order_id), 0)), since a metric expression cannot refer to other metrics (Metric Scoping in core-spec/spec.md)	m5/m5_metric_ref.ossie.yaml
M6	date and time scalars: YEAR, DATE_TRUNC, part-first DATEDIFF(part, start, end)	m6/m6_date_scalars.ossie.yaml

manifest.yaml gets one entry per rung (and the two M3 variants), and the README table lists M0-M6. Each rung lists its elements_under_test with one kind (metric or field) and the name the grader reports it under (a field as dataset.field); a variant whose metrics are dataset-scoped says so with metrics_in: sales.

Bare variants (M0, M2, M3) are dataset-scoped metrics now. Since #486 a model-scoped metric must qualify dataset.field, so the bare form moves to datasets[].metrics, where the single-part name is the required form. The grader read only the model-root metrics and skipped dataset-scoped ones entirely (and the identity test still passed), so it now collects datasets[].metrics as well, keyed dataset.metric as the spec references them; the identity test asserts every metric in a fixture is graded. M6 writes DATEDIFF(day, ...), the spec's spelling.

M7 and M8 (window and offset functions) are not included: they wait for the default NULL placement in window ORDER BY (#587).

Validation

• All 13 fixtures pass validation/validate.py on current main with no warnings (including the reference checks from feat(validation): check metric references and field shadowing against Name Resolution #608).
• The grader's tests pass (18). The identity test runs M0-M6 through a no-op converter: every element grades PORTABLE and every metric is graded. Two manifest tests check that every listed element is found under the grader's own key (_collect) in each variant, and that each primary variant uses the rung's primary reference form. Three tests cover a dataset-scoped metric graded PORTABLE, dropped, and moved to the model root (REWRITTEN).

Related Issues

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

[MrBlack1995]

@kayemkim would you be able to approve the workflows and take a look when you have a moment? Happy to adjust anything.

[jbonofre]

LGTM! Thanks @MrBlack1995!

I checked out the branch and ran the grader test and validate.py on the new and changed M3-M6 fixtures, including SQL validation, and all pass. I also graded the M4-M6 fixtures against themselves, and every finding comes back PORTABLE.

I see two potential follow-ups:

• test_identity_on_real_m0_m3_fixtures_is_all_portable only loops over m0-m3, so the new M4-M6 fixtures are not covered by the regression test. Please add "m4", "m5", "m6" to the loop and update the "M0-M3" wording in grader/README.md.
• M5 has reference_form_primary: qualified, but its only variant is reference_form: metric-name.
• M6's element_under_test is kind: fields and metric with a comma-joined name, so lookups by kind and name won't find it. Splitting it into separate elements, or picking one primary element, would fix that.

[MrBlack1995]

Thanks a lot for checking it out and running everything! All three points are addressed:

• The identity test now covers M0-M6, and the grader README says so.
• M5's reference_form_primary and M6's element entry: every rung now lists elements_under_test, each with one kind (metric or field) and one name, so M6 names its two fields and its metric separately (M2 had the same problem with kind: metrics and no name, so it lists its six metrics now). Two new tests keep this honest: every listed element is found by kind and name in each variant, and each primary variant uses the rung's primary reference form. Both fail on the previous manifest.

While checking M5 against current main I found one more thing: since #608, validate.py warns that M5's total_amount / order_count refers to other metrics, which the spec does not allow (Metric Scoping; the metric_name row there is how a query addresses a metric, not what an expression may contain). So M5 now carries the composition the way a converter has to emit it: the parts inlined, SUM(sales.amount) / NULLIF(COUNT(sales.order_id), 0). With that its primary reference form is qualified again, and it validates without warnings. Happy to change it back or drop M5 if you see it differently.

One note for later, not for this PR: since #486 and #608, the bare variants of M0, M2 and M3 get "is not qualified" warnings, because a model-scoped metric must use dataset.field. I'd suggest a small follow-up that moves the bare form into dataset-scoped metrics, where the unqualified name is the required form.

[MrBlack1995]

@jbonofre thanks again for the review and for running the fixtures yourself, the follow-ups made the manifest a lot more useful. Pushing them dismissed your approval, so whenever you have a moment for another look: the changes are the two commits after 9e425fe, summarized in my comment above. Thanks!

[MrBlack1995]

One more commit since my last note: 4baea94 drops ELSE 0 from the two M3 CASE fixtures, so they equal the FILTER variants as the spec's note under FILTER (WHERE ...) says (with ELSE 0, rows that exist but do not match gave 0 instead of NULL). Found by an execution check on apache/ossie-converters#24; description updated.

[jbonofre]

Thanks @MrBlack1995! I'm doing a new pass 😄

[jbonofre]

This metric sits in the model-root metrics. Since #486, the spec says model-scoped metric expressions must qualify field names as dataset.field, "even when only one dataset has a field with that name" (spec.md, Metric Scoping rule 3 and the Name Resolution examples).
So SUM(amount) FILTER (WHERE status = 'completed') is not a valid document here, even though validate.py lets it through. A converter that follows the spec and rejects or requalifies it would be scored REJECTED or REWRITTEN for doing the right thing.

With #343 closed, bare references are valid only in dataset-scoped metric (datasets[].metrics).
Could we move the bare variants there, under sales? The grader's _collect and the test's _names only read root metrics today, so they would need to pick up dataset-scoped metrics as well.
The same applies to m3_filtered_agg_bare.ossie.yaml, which this PR also edits.
The M0/M2 bare variants already on main have the same problem.
I'm fine with fixing those in follow-up.

[MrBlack1995]

Agreed, and I had the follow-up ready, so it is in this PR now (f0ae6ca) rather than later: all four bare variants (M0, M2 and both M3) carry their metrics under sales in datasets[].metrics, and validate on main without the "is not qualified" warnings. The grader's _collect now also reads dataset-scoped metrics, keyed sales.<metric> as the spec references them (it skipped them before, and the identity test still passed, so that test now also asserts every metric was graded). Three new tests: a dataset-scoped metric graded PORTABLE, dropped, and moved to the model root (REWRITTEN).

[jbonofre]

The spec writes the date part as a bar keyword: DATEDIFF(day, start_date, end_date) (expression_language.md Date Functions).
DATEDIFF('day', ...) with quotes is the Tableau spelling in the Date Function Mapping table.
A converter that returns the spec form would be graded REWRITTEN ("normalized trees differ"), because sqlglot parses 'day' as a string literal and day as a column.

Suggested change

	expression: AVG(DATEDIFF('day', sales.order_date, sales.ship_date))
	expression: AVG(DATEDIFF(day, sales.order_date, sales.ship_date))

DATE_TRUNC('month', order_date) is fine as it is, since the spec quotes the part there.

[MrBlack1995]

Thanks, fixed in 008c947: AVG(DATEDIFF(day, sales.order_date, sales.ship_date)). DATE_TRUNC('month', ...) stays quoted, as the spec writes it.

[jbonofre]

The grader keys fields as dataset.field, but the manifest names them bare: order_year and order_month_start here, and amount_with_tax in M1.
Anyone matching manifest elements to report.findings by (kind, name) won't find a result for any M1 or M6 field.

The new test doesn't catch this because _names in test_grade.py builds its own bare-name set instead of using the grader's keys.

Could we name fields the way the grader does (sales.order_year and so on) and have the test reuse _collect, so the two can't drift apart? For example:

def _names(document, kind):
    return {name for (element_kind, name) in grade._collect(document) if element_kind == kind}

[MrBlack1995]

Done in 008c947: fields are named as the grader reports them (sales.amount_with_tax, sales.order_year, sales.order_month_start), and _names is now your version on top of grade._collect. Since the bare variants now hold dataset-scoped metrics (sales.total_amount in the grader), those four variants say metrics_in: sales, and the test expects <dataset>.<metric> for them. The test fails on the previous manifest; 18 grader tests pass.

[jbonofre]

@MrBlack1995 I'm back on this one 😄

[jbonofre]

The grader can't tell this expression apart from the two ways a converter is most likely to break it.

_equivalent_tree drops every column table qualifier before comparing. Until now every rung had a single dataset, so that only erased spelling. Here customer_id is declared by both sales and customers, so the qualifier carries meaning. Against main grader, all three of these come back REEWRITTEN "cosmetic (normalized trees equal)":

• SUM(sales.amount) / COUNT(DISTINCT customers.customer_id): as authored.
• SUM(sales.amount) / COUNT(DISTINCT sales.customer_id): join collapsed, now also counts customer ids with no customer row.
• SUM(amount) / COUNT(DISTINCT customer_id): unqualified, ambiguous at model scope (validate.py only warns).

interop/portability.py in ossie-converters counts a "cosmetic" REWRITTEN as portable. So a converter that loses the cross-dataset reference scores as portable on the rung that exists to measure it. _repair_by_expression has the same blind spot: a metric that is renamed and also re-pointed to sales.customer_id is paired as "expression equivalent".

To fix that, I suggest to keep the qualifier for any field name that more than one dataset in the input declares, and strip it as before otherwise. M0-M3 each have one dataset, so their grades don't change.

def _ambiguous_fields(document):
    """Field names declared by more than one dataset. For these the dataset
    qualifier says which column is meant, so normalization must keep it."""
    owners = {}
    if isinstance(document, dict):
        for dataset in document.get("datasets") or []:
            if not isinstance(dataset, dict):
                continue
            for field_def in dataset.get("fields") or []:
                if isinstance(field_def, dict) and field_def.get("name"):
                    owners.setdefault(str(field_def["name"]).lower(), set()).add(dataset.get("name"))
    return frozenset(name for name, datasets in owners.items() if len(datasets) > 1)

In _equivalent_tree(left, right, keep_qualified=frozenset()):

        for column in tree.find_all(exp.Column):
            if column.name.lower() in keep_qualified:
                # Declared by more than one dataset: the qualifier is meaning.
                continue
            # Drop the table qualifier so sales.amount and amount compare equal.
            column.set("table", None)

then compute keep_qualified = _ambiguous_fields(input_document) once in grade() and pass it through _classify_present and _repair_by_expression to both _equivalent_tree calls. I ran this against main grader: the existing tests still pass, and the M4 cases come back as review.

[MrBlack1995]

Thanks, good catch: M4 is the first rung where the qualifier carries meaning, and the grader erased it. Applied your approach in 3505677: _ambiguous_fields() reads the field names more than one input dataset declares, grade() computes it once and passes it through _classify_present and _repair_by_expression to both _equivalent_tree calls. M0-M3 grade as before (one dataset each). I also checked the converters side: interop/portability.py in ossie-converters#141 and the #65 runner both go through this grader and do no comparison of their own, so they pick the fix up with the next submodule bump.

[jbonofre]

Could we add regression tests for M4 with the fix? On the current grader, the first and third tests below fail, and with the change above all three pass. The second makes sure that requoting a shared field still counts as as cosmetic, and that amount (declared by only one dataset) still loses its qualifier.

def _m4_document(metric_expression="SUM(sales.amount) / COUNT(DISTINCT customers.customer_id)"):
    def field(name):
        return {"name": name, "expression": {"dialects": [{"dialect": "ANSI_SQL", "expression": name}]}}

    return {
        "datasets": [
            {"name": "sales", "fields": [field("amount"), field("customer_id")]},
            {"name": "customers", "fields": [field("customer_id")]},
        ],
        "metrics": [_metric("revenue_per_customer", "ANSI_SQL", metric_expression)],
    }


def _m4_metric_finding(returned_expression):
    report = grade.grade(_m4_document(), _m4_document(returned_expression))
    return next(f for f in report.findings if f.name == "revenue_per_customer")


def test_a_field_shared_by_two_datasets_keeps_its_qualifier():
    # customer_id is declared by sales and customers, so the dataset is meaning.
    for returned in (
        "SUM(sales.amount) / COUNT(DISTINCT sales.customer_id)",
        "SUM(amount) / COUNT(DISTINCT customer_id)",
    ):
        finding = _m4_metric_finding(returned)
        assert finding.outcome == grade.REWRITTEN, returned
        assert finding.detail.startswith("review"), (returned, finding.detail)


def test_respelling_a_shared_field_is_still_cosmetic():
    finding = _m4_metric_finding('SUM("sales"."amount") / COUNT(DISTINCT "customers"."customer_id")')
    assert finding.detail.startswith("cosmetic"), finding.detail
    # amount belongs to one dataset only, so its qualifier is still spelling.
    finding = _m4_metric_finding("SUM(amount) / COUNT(DISTINCT customers.customer_id)")
    assert finding.detail.startswith("cosmetic"), finding.detail


def test_a_renamed_metric_moved_to_another_dataset_is_not_repaired():
    returned = _m4_document()
    returned["metrics"] = [_metric("rpc", "ANSI_SQL", "SUM(sales.amount) / COUNT(DISTINCT sales.customer_id)")]
    outcomes = {f.name: f.outcome for f in grade.grade(_m4_document(), returned).findings}
    assert outcomes["revenue_per_customer"] == grade.DROPPED_SILENT
    assert outcomes["rpc"] == grade.ADDED

[MrBlack1995]

@jbonofre Added all three in 3505677. On the previous grader the first and third fail, as you said; with the fix all 21 grader tests pass, and all 13 fixtures still validate against current main (8437d59). #700 (M7/M8, stacked on this one) has the fix merged in too: 24 grader tests pass there.