fix(hubs): stop price transform row fan-out and fix eligibility scoping

[Michael Flanakin (flanakin)]

Root-cause findings

Both issues trace back to a shared mechanism (per-invocation partial visibility of Prices_raw) but manifest as two distinct bugs, both present in Prices_transform_v1_0()/Prices_transform_v1_2() (src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Analytics/scripts/IngestionSetup_v1_0.kql, IngestionSetup_v1_2.kql):

Prices_transform_v1_0/v1_2 are Data Explorer update policy functions. When a pricesheet export lands as multiple parquet-snappy files, each file triggers a separate invocation of the update policy, so Prices_raw as seen inside the function during any one execution is only that file's subset of rows (confirmed root cause of #1625).

#1736 (row inflation) — confirmed, distinct mechanism from the eligibility gap:

The savings plan enrichment step:

| lookup kind=leftouter (prices | where x_SkuPriceType == 'Consumption' | where x_SkuMeterId in (spMeters) | distinct tmp_SavingsPlanKey, ListUnitPrice, ContractedUnitPrice, x_BaseUnitPrice) on tmp_SavingsPlanKey

tmp_SavingsPlanKey = strcat(x_SkuMeterId, x_SkuProductId, x_SkuId, x_SkuTier, x_SkuOfferId) — it does not include region, billing profile, or currency. Azure meter prices vary by region/currency, so the same key can legitimately have multiple Consumption rows with different ListUnitPrice/ContractedUnitPrice/x_BaseUnitPrice. distinct over those columns does not guarantee one row per key, so the dimension side of the lookup can have >1 row per tmp_SavingsPlanKey, and lookup kind=leftouter fans out every matching SavingsPlan row on the left — exactly the guideline this repo's docs-wiki/Coding-guidelines.md already documents (distinct Key, Col1, Col2 fans out; use summarize take_any(...) by Key instead). This reproduces within a single update-policy invocation whenever one file contains multi-region/multi-currency Consumption rows for the same meter+product+SKU — it doesn't strictly require the per-file scoping bug, though more files per invocation increases the chance of collisions.

Fix: summarize take_any(ListUnitPrice), take_any(ContractedUnitPrice), take_any(x_BaseUnitPrice) by tmp_SavingsPlanKey on the dimension side, guaranteeing one row per key.

#1625 (eligibility correctness) — confirmed and fixed, not just documented:

let riMeters = prices | where x_SkuPriceType == 'ReservedInstance' | distinct x_SkuMeterId;
let spMeters = prices | where x_SkuPriceType == 'SavingsPlan' | distinct x_SkuMeterId;
...
| extend x_CommitmentDiscountSpendEligibility = iff(x_SkuMeterId in (riMeters) and x_SkuPriceType != 'ReservedInstance', 'Eligible', 'Not Eligible')
| extend x_CommitmentDiscountUsageEligibility = iff(x_SkuMeterId in (spMeters), 'Eligible', 'Not Eligible')

This self-references prices (derived from Prices_raw) to build riMeters/spMeters, which only contain meters visible in the current invocation. If a meter's Reservation/SavingsPlan row lands in one file and its Consumption row lands in another, they're evaluated in separate invocations and eligibility comes out wrong — this is the literal mechanism discussed in #1625.

Fix (option 3 from the #1625 discussion — already half-built): src/open-data/CommitmentDiscountEligibility.csv already exists (added in #2164, refreshed weekly from the Azure Retail Prices API, MeterId confirmed unique — 0 duplicates checked) with exactly the two eligibility columns the transform computes. It just wasn't wired into the hub database yet. This PR:

• Adds a CommitmentDiscountEligibility ADX table (IngestionSetup_HubInfra.kql), alongside the existing PricingUnits/Regions/ResourceTypes/Services reference tables.
• Adds an .set-or-replace ... externaldata(...) pipeline activity in app.bicep to load the CSV, following the exact same pattern as those tables (same dependency chain position, same command shape).
• Replaces the riMeters/spMeters self-referencing logic with lookup kind=leftouter (CommitmentDiscountEligibility) on x_SkuMeterId, dimension side deduped with summarize take_any(...) by MeterId per the join/lookup guidelines. Eligibility is now global (not per-invocation-partial), so this closes the root cause of Prices_transform_v1_0 function does not work as intended #1625, not just a symptom.

The RI-exclusion semantics (x_SkuPriceType != 'ReservedInstance' — a Reservation-priced row is never itself "eligible for reservation") and the "unmatched meter defaults to Not Eligible" behavior are both preserved.

What changed

• src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Analytics/scripts/IngestionSetup_v1_0.kql, IngestionSetup_v1_2.kql: fixed the savings-plan lookup fan-out; replaced self-referencing eligibility calc with a lookup against CommitmentDiscountEligibility.
• src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Analytics/scripts/IngestionSetup_HubInfra.kql: added the CommitmentDiscountEligibility table schema.
• src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Analytics/app.bicep: added the Update CommitmentDiscountEligibility in ADX pipeline activity, spliced into the existing dependency chain (after Update Services in ADX, before Ingestion Complete).
• docs-mslearn/toolkit/changelog.md: unreleased entries for both fixes.

Verification

• bicep build on the modified app.bicep — compiles cleanly.
• Invoke-Pester -Path src/powershell/Tests/Lint/KqlJoinKinds.Tests.ps1 — 436/436 passed (no bare joins, no ARG-rejected operators introduced).
• Invoke-Pester -Path src/powershell/Tests/Unit/HubsKqlOperators.Tests.ps1 — 142/142 passed.
• Invoke-Pester -Path src/powershell/Tests/Unit/HubsIngestionQueries.Tests.ps1,src/powershell/Tests/Unit/HubsContractedCostGuard.Tests.ps1,src/powershell/Tests/Unit/HubsAdfTriggerTimeZones.Tests.ps1 — no failures.
• Manually confirmed table-creation ordering: ingestion_VersionedScripts (which deploys the transform functions) dependsOn ingestion_InitScripts (which deploys IngestionSetup_HubInfra.kql, creating CommitmentDiscountEligibility before it's referenced).

Open questions / risks / scope not covered

• Did not attempt option 1 or 2 from the Prices_transform_v1_0 function does not work as intended #1625 discussion (changing ingestion to process all files at once, or a post-ingestion .update command) — the open-data lookup is less invasive and matches RolandKrummenacher's preferred direction.
• Prices_transform_v1_0 is marked DEPRECATED in its own docstring ("Use Prices_transform_v1_2() instead"), but issue [Hubs] Price ingestion creates extra rows #1736's reported counts were specifically against Prices_transform_v1_0, so I fixed both versions for consistency; happy to drop the v1_0 change if it's considered out of scope for a deprecated path.
• The CommitmentDiscountEligibility open-data CSV depends on the Azure Retail Prices API weekly refresh (Update-CommitmentDiscountEligibility.ps1) being current; a brand-new meter that hasn't been picked up by that refresh yet would default to "Not Eligible" until the next weekly run, versus the old logic which (when it worked) reflected same-day eligibility from the pricesheet export itself. This is a minor freshness trade-off in exchange for correctness/completeness.
• Did not add new Pester coverage asserting the eligibility values end-to-end (e.g., a fixture-driven test of Prices_transform_v1_2() output) — no existing test harness runs the KQL transform logic itself (only lint/static checks), so this would be new test infrastructure; flagging as a possible follow-up rather than in scope here.

Fixes #1736
Refs #1625

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Michael Flanakin (flanakin)]

Reviewed via a multi-angle code review pass (correctness, removed-behavior, cross-file/deployment tracing, reuse/simplification). Summary:

Core fix is sound. The row-fan-out fix (take_any on the SavingsPlan dimension side) and the eligibility-scoping fix (sourcing from the CommitmentDiscountEligibility open-data table instead of self-referencing the per-invocation prices table) both correctly address #1736 and #1625. Verified against the official Kusto lookup operator docs that project-away MeterId after the equality-by-value lookup is safe — the join-key columns aren't repeated in lookup output, so there's no MeterId/MeterId1 collision (one review pass flagged this as a possible compile break; traced it and confirmed it isn't).

Fixed in e9f42774:

• Case-sensitivity gap: the new lookup joined x_SkuMeterId (raw from the pricesheet export, casing undocumented by Cost Management's schema) against CommitmentDiscountEligibility.MeterId (forced lowercase by Update-CommitmentDiscountEligibility.ps1) using case-sensitive equality. lookup on has no case-insensitive variant, so both sides are now normalized with tolower() before the join. Silent failure mode if left unfixed: any casing mismatch defaults affected meters' eligibility to 'Not Eligible' with no error.
• Retry/timeout: CommitmentDiscountEligibility.csv (~92K rows, ~5.5MB) is ~8x larger than the next-biggest reference CSV this pattern was copied from (ResourceTypes.csv). Bumped retry: 0 → 2 and commandTimeout: 20min → 30min to match the load size.

Not in scope for this PR, flagged for follow-up:

• Init-DataFactory.ps1's Invoke-AzDataFactoryV2Pipeline call is fire-and-forget (no run-status polling), so on a fresh deploy the ARM deployment script can complete before pipeline_InitializeHub (including the new eligibility table load) actually finishes. This is a pre-existing pattern shared by every reference table, not introduced here — but this PR is the first case where it can silently produce wrong eligibility data rather than just missing decorative labels. Worth its own issue.
• MeterId uniqueness in Update-CommitmentDiscountEligibility.ps1's output is structurally guaranteed (hashtable-keyed) but not asserted by a test — minor, low-risk cleanup opportunity.

Verified after the fix: bicep build clean, 578/578 Pester tests pass (KqlJoinKinds, HubsKqlOperators).

[Roland Krummenacher (RolandKrummenacher)]

Review — PR #2246

The root-cause analysis holds up on both counts, and I verified the supporting claims: the "~8x larger than ResourceTypes" figure is right (5.7 MB vs 718 KB, 92,283 rows), Package-Toolkit.ps1:239 picks the CSV up via its *.csv wildcard so no packaging change is needed, the open-data.md#commitment-discount-eligibility anchor resolves, and the ingestion_InitScripts → ingestion_VersionedScripts ordering claim is correct.

Two findings I'd treat as blocking, one substantive, and one thing I want to defend pre-emptively so it doesn't get bounced. Details inline; summary:

1. Blocking — local hubs silently get 100% "Not Eligible". dev gained IngestionSetup_OpenDataExternal.kql in #2187, merged after this branch forked. It's the local/emulator population path for the open-data tables. This PR adds a fifth table but populates it only through the new ADF activity, which local hubs never run.
2. Blocking — Azure Government and China hubs regress to 100% "Not Eligible". The generator reads the public-cloud retail catalog only; those clouds' meter IDs aren't in it. The current self-derived logic works there precisely because it reads the customer's own pricesheet.
3. Substantive — take_any replaces a loud bug with a quiet one. Row inflation is genuinely fixed, but the arbitrary pick can stamp one billing profile's negotiated price onto another's savings plan rows.
4. Not a problem — the tolower() normalization is correct and does not violate the repo KQL rule. Flagging so a later reviewer doesn't reject it on a false positive.

On the two open questions in the description: keeping the v1_0 change is the right call in my view — deprecated or not, it's what #1736 was reported against, and leaving the two versions divergent costs more than it saves. And I agree the open-data lookup beats options 1 and 2; my objection is to its current failure modes, not the approach.

Coordination: #2251 rewrites Update-CommitmentDiscountEligibility.ps1 and renames .shardcounts.json → .familycounts.json. No file overlap with this PR, but this PR makes hub ingestion depend on that CSV's completeness, which raises the stakes on the traversal correctness considerably. I'd land #2251 first.

The CONFLICTING state is only changelog.md — trivial.

[Roland Krummenacher (RolandKrummenacher)]

Blocking — local hubs will silently report every meter as "Not Eligible".

dev gained IngestionSetup_OpenDataExternal.kql in #2187 ("Run FinOps hubs on your own hardware"), which merged after this branch forked — so it isn't visible in this diff. It's the local/emulator stand-in that populates the open-data tables, built into the finops-hub-local-opendata.kql release artifact via .build.config and consumed by Initialize-FinOpsHubLocal.ps1:181.

This PR adds a fifth open-data table, creates it here, and populates it only through the new Update CommitmentDiscountEligibility in ADX pipeline activity. Local hubs never run Data Factory. So the table exists, empty, the lookup in Prices_transform_v1_0/v1_2 misses every meter, and both eligibility columns come out 'Not Eligible' for 100% of rows — with no error anywhere.

The fix is one line added to IngestionSetup_OpenDataExternal.kql, mirroring the four already there:

.set-or-replace CommitmentDiscountEligibility <| externaldata(MeterId: string, x_CommitmentDiscountSpendEligibility: string, x_CommitmentDiscountUsageEligibility: string)[@'$$openDataPath$$/CommitmentDiscountEligibility.csv'] with (format='csv', ignoreFirstRecord=true)

This will surface when you merge dev in to clear the changelog conflict, but it's worth catching deliberately rather than discovering the empty table later.

---

Minor, same area — Fabric upgrade path. ingestion_InitScripts and ingestion_VersionedScripts are both if (useAzure) (app.bicep:357/376), so Fabric users run these scripts by hand. Prices_transform_v1_2() now references CommitmentDiscountEligibility, so a Fabric user who reruns only the versioned script against an existing eventhouse hits a function-validation failure on a table that doesn't exist yet. Worth an explicit "run the infra script first" note in the upgrade guidance.

[Roland Krummenacher (RolandKrummenacher)]

Blocking — this regresses Azure Government and Azure China hubs to 100% "Not Eligible".

Update-CommitmentDiscountEligibility.ps1:63 fetches https://prices.azure.com/api/retail/prices with meterRegion 'primary' — the public-cloud retail catalog. Gov and China meter IDs don't appear there, so for a hub ingesting a Gov or China pricesheet this lookup matches nothing and both eligibility columns become 'Not Eligible' for every row.

Today's riMeters/spMeters logic works in those clouds precisely because it reads the customer's own pricesheet rather than a public catalog. It's wrong under the multi-file split you diagnosed, but it isn't uniformly wrong — this would be.

The description lists "unmatched meter defaults to Not Eligible" as preserved behavior, which is mechanically true, but that was a rare tail case before and becomes the only outcome for entire clouds. Options as I see them:

• Keep the self-derived calc as a fallback when the lookup misses — coalesce against the old iff(... in (riMeters) ...) rather than defaulting straight to 'Not Eligible'. Preserves today's behavior wherever the open data can't reach, and the open data wins wherever it has an answer.
• Or gate on cloud and document the limitation explicitly.

The first looks strictly better to me — it also covers the new-meter freshness gap you flagged in the description, since a meter the weekly refresh hasn't picked up yet would fall back to the pricesheet-derived answer instead of silently reading "Not Eligible".

[Roland Krummenacher (RolandKrummenacher)]

Not a problem — flagging pre-emptively so this doesn't get bounced on a false positive.

This tolower() is correct and does not violate the repo's KQL rule. The guideline targets tolower() in comparison position, where the operator is already case-insensitive. lookup ... on only does case-sensitive equality and has no =~ equivalent, so normalizing both sides of the join key is the right call — the alternative would be a join on $left.k =~ $right.k, which is worse on every axis.

HubsKqlOperators.Tests.ps1:91-92 only matches tolower() adjacent to a comparison operator, so the lint pass reported in the description is a true pass, not a gap in the rule.

[Roland Krummenacher (RolandKrummenacher)]

Substantive — this fixes the row count but can silently assign the wrong price.

The fan-out is real and take_any does guarantee one row per key. But the description already identifies the actual root cause: tmp_SavingsPlanKey = strcat(x_SkuMeterId, x_SkuProductId, x_SkuId, x_SkuTier, x_SkuOfferId) carries no billing profile, currency, or region. When that key legitimately has several Consumption rows with different prices, take_any doesn't resolve the ambiguity — it picks arbitrarily and hides it.

That matters because a hub can ingest pricesheets for multiple billing accounts and profiles into one Prices_raw, and Prices_final keeps x_BillingAccountId, x_BillingProfileId, and PricingCurrency per row. Two enrollments with different negotiated rates for the same meter+product+SKU+tier+offer collide on this key, and a savings plan row can end up carrying the other enrollment's ContractedUnitPrice. That propagates into x_ContractedUnitPriceDiscount, x_EffectiveUnitPriceDiscount, and both ...DiscountPercent columns, so the discount math on savings plan rows goes wrong in a way nothing downstream can detect.

Before: loud and detectable — row counts exceed Prices_raw, which is how #1736 got reported in the first place. After: correct row counts, quietly wrong numbers. That's arguably the worse failure mode.

The fix that matches the diagnosis is widening the key — adding x_BillingProfileId and PricingCurrency, plus x_SkuRegion if regional collisions are real for a given meter — and then keeping take_any on top as the cheap defence it's meant to be. Worth noting that the repo guideline preferring take_any over distinct assumes the key is genuinely unique; this PR establishes that it isn't.

If widening the key is out of scope here, I'd rather see that stated as a deliberate decision with a follow-up issue than left as an arbitrary pick.

---

Minor, same line — the semi-join prefilter is gone. The old dimension side had | where x_SkuMeterId in (spMeters), which had to go along with spMeters. The summarize now aggregates every Consumption row in the invocation instead of only those for savings-plan meters. Results are identical, but it's more work on the ingestion hot path. Recoverable with an inline subquery if it shows up in practice:

| where x_SkuMeterId in ((prices | where x_SkuPriceType == 'SavingsPlan' | distinct x_SkuMeterId))

[Roland Krummenacher (RolandKrummenacher)]

Same as the v1_2 comment: the take_any here fixes the row count but can silently pick another billing profile's or currency's price, and the | where x_SkuMeterId in (spMeters) prefilter is gone from the dimension side. Whatever you settle on for v1_2 should apply here too — I agree with keeping the two versions in sync rather than dropping the deprecated path.