[codex] Add dimensional forecast point fields#5061
Conversation
There was a problem hiding this comment.
LGTM. Additive optional fields on ForecastPoint, minor changeset is the right call. Follow-ups below.
Things I checked
forecast-vendor-metric-value.jsonis a clean ForecastRange-swapped twin ofcore/vendor-metric-value.json— same envelope, same required set[vendor, metric_id, value], sameadditionalProperties: false. Onlyvalueandmeasurable_impressionschange shape.forecast-point.json:48-95viewabilitymirrorsdelivery-metrics.json:284-318field-for-field with ForecastRange swaps.additionalProperties: truematches the delivery side.oneOfinforecast-point-dimensions.jsonis disjoint: each branch gated bykindconst +additionalProperties: false. Discriminator pattern thatscripts/audit-oneof.mjsaccepts — no baseline regression.kindvalues (geo,placement,device_type,device_platform,audience) line up 1:1 with the existingreporting_dimensionskeys inmedia-buy/get-media-buy-delivery-request.json:96,132,149,166,184. The doc's post-purchase reconciliation claim is load-bearing.- Buyer workflow mapping in
media-products.mdxis grounded:targeting_overlay.geo_countries/geo_regions/geo_metros/geo_postal_areas,audience_include, device overlay fields,creative_assignments[].placement_refsall exist on the schemas referenced. x-entity: "placement"registration is correct — compositepublisher_domain + placement_idnamespace, not product-scoped. Applied consistently inplacement-ref.jsonandplacement.json.viewable_ratelower bound inherited fromforecast-range.jsonminimum: 0; theallOfonly needs to addmaximum: 1. No drift.- Changeset is
minor. Base schema already hadadditionalProperties: true; new fields are all optional. Right type.
Follow-ups (non-blocking — file as issues)
systemis free-form on forecast geo rows.forecast-point-dimensions.json:21-24typessystemas bare"type": "string", but the reporting-side counterpart atmedia-buy/get-media-buy-delivery-request.json:105-114usesoneOf(metro-system, postal-system), andcore/targeting.json:50-52,98-101$refs those same enums. Forecast rows can emit asystemvalue buyers cannot reconcile against targeting/reporting. Tighten to the sameoneOffor parity.- Forecast viewability requires
standard; delivery viewability does not.forecast-point.json:87-95enforces "any of measurable_impressions / viewable_impressions / viewable_rate / viewed_seconds present →standardrequired."delivery-metrics.json:284-318carries no equivalent invariant. The stricter forecast-side rule is defensible (MRC vs GroupM rows are not interchangeable, as the doc says), but the asymmetry means a row that round-trips forecast → delivery loses the guarantee. Consider whether to backport the invariant to delivery-metrics. - The
if/thenrequiringsystemfor metro/postal_area is stricter here than at peer sites.forecast-point-dimensions.json:35-50enforces conditionalsystemrequirement. The same constraint is prose-only atget-media-buy-delivery-response.jsonby_geoandcore/product-filters.jsonrequired_geo_targeting. Either backfill the conditional there or note the asymmetry.
Minor nits (non-blocking)
- Empty
viewability: {}validates.forecast-point.json:87-95— thenot(anyOf required)branch accepts an empty object. Probably intended (same effect as omitting the field), but worth a one-line schema comment so a future reader doesn't read it as a bug. viewable_rate.allOf[1]lacks"type": "object".forecast-point.json:65-74— works in practice because the only value reaching that constraint is already an object, butdelivery-metrics.jsonstatesminimum: 0andmaximum: 1inline as one block. The split-across-allOf form is more clever than it needs to be.
Approving on the strength of the dimensional-row design (one product, many rows beats product-per-country fanout) plus the mirror discipline between forecast and delivery shapes. Safe to merge.
|
Non-blocking follow-up notes from review:
|
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Right shape for what this is solving — dimensions as a kind-discriminated array lets one product expose placement×country availability without product-per-slice expansion, and the canonical-id parity with reporting dimensions and targeting overlays is intact.
Things I checked
forecast-point-dimensions.jsononeOfuseskindwithconstvalues across all five variants andrequired: ["kind", ...]on every branch. Classifies asdiscriminatedperscripts/audit-oneof.mjs:188-215— no baseline entry needed, and absent fromscripts/oneof-discriminators.baseline.json.- Canonical identifier parity:
geo_level/geo_code/system,device_type,device_platform,audience_id+audience_sourceall match the reporting-dimensions block inget-media-buy-delivery-response.jsonand the targeting overlay surfaces.placement_refmatchescreative_assignments[].placement_refs. viewabilitymirror parity vsdelivery-metrics.viewabilityatdelivery-metrics.json:284-318. New forecast version adds the ratchet atforecast-point.json:87-95requiringstandardwhenever any rate field is present — branch A (no rate field) and branch B (standard present) correctly cover the cases.viewable_rateallOfatforecast-point.json:64-76correctly boundslow/mid/highto[0, 1]via override of the baseforecast-range.jsonnumeric range.forecast-vendor-metric-value.jsonmirrorsvendor-metric-value.json— same required[vendor, metric_id, value], sameadditionalProperties: false,breakdownpassthrough preserved. Only delta isvalueandmeasurable_impressionsupgraded toForecastRange. Right call.- Geo
if-thenrequiringsystemwhengeo_levelismetroorpostal_areaworks givengeo_levelis required at top level. - Docs example at
media-products.mdx:1094-1166validates against the schemas. Both example points includestandard: "mrc"whenever a viewability rate field is emitted — the new ratchet is honored in the doc itself. - Buyer-workflow mapping at
media-products.mdx:1170-1176:creative_assignments[].placement_refsas creative-routing-only is consistent withcreative-assignment.json— placement narrowing happens at package targeting time, not at creative assignment. - Changeset
minoris correct: every new field is optional and additive. No required-field flips, no enum removals, no shape changes to existing surfaces. x-entity: placementannotation on bothplacement.json:16andplacement-ref.json:16follows the established pattern (annotate every site carrying the identifier, dedup is the lint's job).
Follow-ups (non-blocking — file as issues)
- Placement dimension shape diverges from delivery
by_placement.forecast-point-dimensions.json:65-72uses a structuredplacement_ref(publisher-scoped), butget-media-buy-delivery-response.jsonby_placementstill emits a bareplacement_idstring. Forecast shape is the better one — multi-publisher-safe and aligned withcreative_assignments[].placement_refs— but the asymmetry means buyer agents can't cross-walk forecast→delivery rows by identical key extraction. Worth a follow-up to upgradeby_placement.placement_idtoplacement_ref, or an explicit note that the asymmetry is deliberate during transition. standard-required ratchet asymmetric vs delivery-metrics.forecast-point.viewabilityrequiresstandardwhen any rate field is present (forecast-point.json:87-95);delivery-metrics.viewabilitykeepsstandardoptional unconditionally (delivery-metrics.json:284-318). Reasonable to enforce up-front at forecast time and rely on the seller carrying the same standard into delivery, but the inconsistency lets a buyer compare a forecasted MRC viewable_rate against an unlabeled delivered viewable_rate. Either tighten delivery-metrics in a separate change or document the divergence as intentional.uniqueItemsdoes not enforce per-kind uniqueness ondimensions.forecast-point-dimensions.json:6-8— two items with the samekindbut different codes (e.g.,geo:USandgeo:CAon one point) deep-compare as distinct and passuniqueItems. Two geos in one intersection is not a meaningful row. Either add a per-kind uniqueness constraint or document in the description that intersection items must each have a distinctkind.audience_idselectability constraint is only in prose. The docs at line 1173 correctly note that informational audience rows don't automatically become targeting handles, butforecast-point-dimensions.json:111-135doesn't carry that constraint into the schema description. Mirror the prose into the schema description so buyer agents reading the schema in isolation don't try to liftkind: "audience"rows intoaudience_include.
Minor nits (non-blocking)
- Viewability
additionalProperties: trueplus openanyOfleaves a typo escape hatch.forecast-point.json:48-96— a payload with a typo likeviewablerate(missing underscore) would pass branch A of theanyOfbecause none of the spelled rate fields are present, sostandardis not enforced. Either tighten toadditionalProperties: falseon the viewability block, or accept that the open shape is intentional for forward-compatible vendor extensions and the typo case is a buyer-side conformance bug. - Empty
placement_idis allowed.placement-ref.json:13-17has nominLengthonplacement_id, so aplacement_ref: { placement_id: \"\" }validates. Pre-existing, not introduced here, but inherited by the new placement dimension variant. - Redundant
type: stringinside the geoif.forecast-point-dimensions.json:42-46— the outergeo_level$refalready constrains to the enum; the nestedtype: stringis harmless but redundant.
Approved.
|
Resolved the follow-up concerns in
Validation and pre-push checks passed; PR body has the full list. |
There was a problem hiding this comment.
Request changes. The dimensional ForecastPoint shape is the right design — array of intersection items with kind as the set key, geo if/then constraining system to the existing metro/postal enums, ForecastRange-shaped viewability and vendor metrics, product_id for proposal-level allocation mapping. That part is clean. The block is the bundled tightening of delivery-metrics.viewability shipped under a minor changeset.
static/schemas/source/core/delivery-metrics.json:318-331 adds an anyOf that requires standard whenever measurable_impressions, viewable_impressions, viewable_rate, or viewed_seconds is present. Published dist/schemas/3.0.12/core/delivery-metrics.json had standard as a plain optional property with no conformance gate. That is an optional→required flip on an existing public field. Per MUST FIX #5 and the playbook semver rules at lines 295-298, an optional→required flip on a published field is a major change. .changeset/5058-forecast-point-dimensions.md:2 declares minor; main is in pre mode so this ships as 3.1.0-beta.N, not 4.0.0. A 3.0-conformant seller emitting viewability: { measurable_impressions: N, viewable_rate: r } without standard becomes schema-invalid against 3.1.x without a major-version migration note. The same wire-break is duplicated in static/schemas/source/compliance/comply-test-controller-request.json:696-708 for simulate_delivery.params.viewability, so any existing harness fixture that omits standard will also break.
I agree that MRC and GroupM rows are materially different and standard SHOULD be present. The data-quality argument doesn't change the semver math.
Fix options
- Split the delivery-metrics tightening into its own
majorchangeset with a migration note. The forecast-point additions stayminor. This is the cleanest path. - Weaken to SHOULD in prose, drop the
anyOffromdelivery-metrics.jsonandcomply-test-controller-request.json. The new forecast-pointviewabilityanyOf atforecast-point.json:91-100is fine to keep — that's a new field, additive. Only the delivery-side tightening is the wire-break.
Either is acceptable. Pick one.
Things I checked
- Geo
if/thencomposes correctly under draft-07.not.required.systemcorrectly rejects{kind:"geo", geo_level:"country", geo_code:"US", system:"us_zip"}; the metro/postal branches' nestedsystem: $refenums layer on top of the variant-levelsystem: {type:string}so metro+us_ziprejects.forecast-point-dimensions.json:42-99. oneOfdiscriminator soundness — every variant declareskindasconstwithrequired:["kind"]and distinct values.scripts/audit-oneof.mjsconst-property path classifies asdiscriminated. No baseline ratchet needed.x-adcp-validation.unique_item_properties: ["kind"]atforecast-point-dimensions.json:6-9is docs-only — AJV won't enforce it. The buyer-sidedimensions sorted by kindrow-identity claim indocs/media-buy/product-discovery/media-products.mdx:984is therefore stronger than the wire enforces.- Docs/schema coherence on the array shape — the JSON examples at
media-products.mdx:1095-1167use thedimensions: [...]array form consistently. The earlier object form from the first commit is gone. product_idsemantics are clear atforecast-point.json:19-23andmedia-products.mdx:1178— populated when a proposal-level dimensional row maps to one allocation, omitted for aggregates spanning multiple products.x-entity: placementadded toplacement-ref.json:16,placement.json:16,x-entity-types.json:50. Composite identity documented in the type entry; matches thevendor_metricprecedent.
Follow-ups (non-blocking — file as issues)
- Test 9 in
tests/schema-validation.test.cjs:444-479is thin. It validates thex-adcp-validationannotation and exercises a hand-rolledhasRepeatedKindhelper. It does not compileforecast-point-dimensions.jsonwith AJV and exercise theif/then/not.required.systemcountry/region branches, the metro/postalsystemenum mismatch rejection, or the new viewabilityanyOfgate against valid/rejected fixtures. The PR description claims those AJV runs happened locally; the fixtures aren't in the repo. Add a fixture-based AJV pass so the conformance gates are CI-enforced. x-adcp-validation.unique_item_propertiesis unenforced at the wire. Either ship a custom AJV keyword that conformance tooling consumes, or update the docs to saykinduniqueness is buyer-side normalization, not a schema-enforced invariant. The current state has docs claiming MUST NOT and a wire that silently passes the violation.
Reblock comes off once the delivery-metrics + comply-test-controller tightening is either pulled out into its own major or weakened to SHOULD.
Summary
Adds dimensional and measurement-aware forecast point fields for #5058.
ForecastPoint.dimensionsas an array of dimension constraints, so a single point can represent intersections such as placement x country, placement x device, or geo x audiencekindvariants for geo, placement, device type, device platform, and audience dimensionssystem, metro usesmetro-system, and postal usespostal-systemkindvalues a conformance error via normative schema/docs language plusx-adcp-validation.unique_item_properties: ["kind"]and test coverageviewabilityranges and forecastedvendor_metric_valuesranges independent ofpricing_optionsviewability.standardwhenever measured viewability values are presentForecastPoint.product_idso proposal-level dimensional rows can map back to executable product allocationsx-entitycoverage and a minor changesetValidation
npm run test:schemasnpm run test:examplesnpm run test:oneof-discriminatorsnpm run test:json-schema -- --file docs/media-buy/product-discovery/media-products.mdxnpm run test:json-schema -- --file docs/media-buy/task-reference/get_products.mdxnpm run build:schemasgit diff --checknpm run test:unit,npm run test:test-dynamic-imports,npm run test:callapi-state-change,npm run typecheckNotes
Expert review feedback was incorporated across protocol, docs, and consumption semantics. The array shape is preserved for readable intersections, while repeated
kindvalues are now a conformance error handled by normative docs and custom validation metadata because JSON Schema draft-07 cannot express unique-by-property for this array form.