[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.

[Michael Flanakin (flanakin)]

🤖 [AI] PR update

• Merged dev (v15 release): resolved changelog/upgrade conflicts, ms.date set to 10/28/2026. Entries moved to a new active Unreleased section.
• Restored the x_SkuMeterId in (spMeters) prefilter on the savings plan lookup dimension side in v1_0/v1_2. The earlier reply said it was restored, but it was missing.
• Added HubsPriceTransformLookups.Tests.ps1. A regex emulation of its patterns fails on dev and passes on this branch. Pester itself could not run in this sandbox.