Skip to content

Add KQL string matching and join/lookup rules to the coding guidelines - #2223

Open
RolandKrummenacher wants to merge 7 commits into
devfrom
docs/kql-coding-guidelines
Open

Add KQL string matching and join/lookup rules to the coding guidelines#2223
RolandKrummenacher wants to merge 7 commits into
devfrom
docs/kql-coding-guidelines

Conversation

@RolandKrummenacher

@RolandKrummenacher RolandKrummenacher commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

🛠️ Description

The coding guidelines cover general practice, content, and the changelog, but say nothing about KQL — the primary language of the hub. docs-wiki/Coding-guidelines.md, AGENTS.md, and CONTRIBUTING.md don't mention KQL or Kusto at all today. That's part of why the redundant tolower() comparisons and word-search contains usages accumulated across the ingestion scripts until #2213 / #2220 — and why bare joins relying on the innerunique default accumulated until #2225. Nothing in the project's written standards contradicted them.

This PR writes those rules down where both humans and AI assistants will find them.

docs-wiki/Coding-guidelines.md — a new ⚡ KQL section with two rule sets:

String matching (from #2213 / #2220):

  • String comparison: why tolower() in comparison position is always wrong (every KQL string operator is already case-insensitive; the _cs forms are the case-sensitive ones), with an avoid/prefer table for the common rewrites — tolower(x) containshas, tolower(x) ===~, tolower(a) != tolower(b)!~, indexof(...) >= 0has, and dynamic operands not needing a tostring() wrapper.
  • When contains is required: framed as a semantic distinction (whole term vs. arbitrary substring) rather than a performance one, with the legitimate cases — needles fused inside a larger token (ConsumedUnit contains 'MB' also matching Mbps), word-stem matching, and fragments that never form a whole term. Also documents the term-index limits for punctuation and sub-3-character needles.
  • Verify before you swap: per-row cross-tabulation on real data rather than aggregate counts (which can hide offsetting false positives and negatives), plus a pointer to the executable equivalence harness added in Use case-insensitive KQL operators instead of tolower() comparisons #2220.

Joins and lookups (from #2225):

  • The innerunique trap: a bare | join deduplicates the left side on the join key — the root cause of real silent data loss fixed in Fix KQL join pitfalls and adopt lookup for dimension enrichment #2225 (savings plan recommendations collapsing to one row per subscription). Explicit kind= is mandatory.
  • Canonical forms by intent: lookup (+ summarize take_any(...) by key dedup — distinct is not a per-key dedup) for dimension enrichment, leftsemi/inner for filtering, leftanti for exclusion (never leftouter + isempty(), which duplicates before it filters), fullouter with key coalescing for period comparisons, and shuffle hints for large-to-large joins.
  • ARG is not ADX: live-probing every flavor gave a matrix that differs from the ARG docs in both directions — supported: inner, innerunique, leftouter, rightouter, fullouter; rejected: lookup, all semi/anti flavors, and in/!in with a subquery. Since leftanti doesn't exist there, the leftouter + isempty() emulation is documented as the one acceptable exclusion form in ARG (with a key-unique right side).

AGENTS.md — both rule sets are also stated inline under Coding Standards. That file already listed Bicep, PowerShell, markdown, and commit conventions but not KQL, and it's what both Claude Code (via CLAUDE.md) and Copilot (via .github/copilot-instructions.md) load. Stating it there means an assistant sees the rule without having to follow the link.

Merge order: the join/lookup rules reference KqlJoinKinds.Tests.ps1, which lands in #2225 — merge #2225 first so the enforcement pointer resolves (the text marks it as "added in #2225" either way).

No behavior change — documentation only. The guidance is already enforced in CI: string rules by HubsKqlOperators.Tests.ps1 (from #2220), join rules by KqlJoinKinds.Tests.ps1 (from #2225). This PR documents the reasoning so contributors know why before the test tells them no.

📋 Checklist

🔬 How did you test this change?

  • 🤏 Lint tests — markdownlint-cli2 with the repo config: 0 issues

📦 Deploy to test?

Not applicable — documentation only.

🙋‍♀️ Do any of the following that apply?

  • 🚨 This is a breaking change.

📑 Did you update docs/changelog?

  • N/A — docs-wiki is contributor documentation, not part of the release changelog. It syncs to the GitHub wiki automatically on merge to dev.

🤖 Generated with Claude Code

The coding guidelines covered general practice, content, and the
changelog, but said nothing about KQL — the primary language of the
hub — so nothing contradicted the tolower()/contains patterns that
accumulated across the ingestion scripts until #2213/#2220.

Add a KQL section to docs-wiki/Coding-guidelines.md covering:
- Why tolower() in comparison position is always wrong (KQL string
  operators are already case-insensitive; _cs are the sensitive ones),
  with an avoid/prefer table for the common rewrites.
- has vs contains as a semantic distinction (whole term vs arbitrary
  substring), not just a performance one, including when contains is
  genuinely required and the term-index limits for short/punctuation
  needles.
- How to verify a swap: per-row cross-tabulation on real data rather
  than aggregate counts, plus the executable equivalence harness.

Also surface the rule directly in AGENTS.md so Claude Code and Copilot
see it without following the link — the file previously listed Bicep,
PowerShell, markdown and commit conventions, but not KQL.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 31, 2026 07:26
@RolandKrummenacher
RolandKrummenacher requested review from a team, MSBrett and flanakin as code owners July 31, 2026 07:26
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Needs: Review 👀 PR that is ready to be reviewed label Jul 31, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Documents project-specific KQL string-matching rules in the contributor coding standards so recurring anti-patterns (e.g., tolower() in comparisons and overuse of contains) are prevented via clear guidance alongside existing CI enforcement.

Changes:

  • Adds a new ⚡ KQL section to docs-wiki/Coding-guidelines.md covering case-insensitive operators, has vs contains, and validation guidance.
  • Adds a concise KQL rule summary to AGENTS.md under Coding Standards so assistants and contributors see it inline.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
docs-wiki/Coding-guidelines.md Introduces detailed KQL string matching guidance (operators, has vs contains, and verification steps).
AGENTS.md Adds an inline KQL standards summary referencing the coding guidelines and existing CI enforcement.
Suppressed comments (1)

docs-wiki/Coding-guidelines.md:65

  • This row states that =~ accepts a dynamic operand directly. That can be true for scalar dynamic values, but it is not universally safe if the field can be an object/array (or otherwise not string-like), and may produce confusing comparisons. Suggest qualifying the guidance to avoid implying tostring() is never needed for dynamic fields.
| `tostring(Dyn.Field) =~ 'true'` | `Dyn.Field =~ 'true'` | These operators accept a dynamic operand directly |

Comment thread docs-wiki/Coding-guidelines.md Outdated
Review flagged that 'these operators accept a dynamic operand directly'
was too absolute for fields that can hold an object or array.

Verified on a live cluster: tostring() is not the missing piece — raw
and tostring()-wrapped comparisons return identical results for every
payload shape, including objects and arrays. The real hazard is that a
non-scalar is compared against its JSON serialization, where has
matches a value nested anywhere in the text:

    dynamic({"nested":"true"}) has 'true'  -> true
    dynamic({"nested":"true"}) =~  'true'  -> false

Scope the table row to scalar dynamic values and add a note describing
the JSON-serialization behavior, why tostring() does not help, and what
to use instead (extract the member, or array_index_of/set_has_element
for arrays). All claims in the note executed against a cluster.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RolandKrummenacher

Copy link
Copy Markdown
Collaborator Author

Good catch on the dynamic-operand row — it was too absolute. Qualified in 150c52d.

Verified the actual behavior on a live cluster first, and the result is a bit different from what the comment assumed: tostring() is not the missing piece. Raw and tostring()-wrapped comparisons return identical results for every payload shape, objects and arrays included:

d.v tostring(d.v) d.v =~ 'true' tostring(d.v) =~ 'true' d.v has 'true' tostring(d.v) has 'true'
"true" (string) true true true true true
true (bool) true true true true true
1 (number) 1 false false false false
{"nested":"true"} (object) {"nested":"true"} false false true true
["true"] (array) ["true"] false false true true
missing key (empty) false false false false

So the real hazard isn't a missing cast — it's that a non-scalar gets compared against its JSON serialization, where has matches a value nested anywhere in the text ({"nested":"true"} has 'true'true, while =~false). Adding tostring() produces the same JSON text and the same surprising result, so recommending it would give false confidence.

The table row is now scoped to scalar dynamic values, with an IMPORTANT note covering the JSON-serialization behavior, why tostring() doesn't help, and what to use instead — extract the member you mean (Dyn.Field.nested), or array_index_of() / set_has_element() for arrays. Every claim in that note was executed against a cluster.

Review flagged that the indexof(Col,'x') >= 0 -> Col has 'x' row was
not an equivalence: indexof() is substring-based while has is
term-based, so presenting it as a mechanical rewrite could lead
contributors into a behavior change. Verifying on a live cluster showed
the row was wrong on a second axis too — indexof() is case-sensitive:

  value            indexof>=0  contains  has
  'Windows Server'    true       true    true
  'WindowsServer'     true       true    false   <- term vs substring
  'MyWindowsBuild'    true       true    false   <- term vs substring
  'windows server'    false      true    false   <- case sensitivity

So indexof(Col,'x') >= 0 is exactly contains_cs, not contains and not
has. Reframe the row to keep the real advice (don't compute a position
you only need as a boolean) while stating both differences and telling
contributors to pick by intent. This mirrors the has-vs-contains
distinction the section already makes, which the original row
contradicted.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread docs-wiki/Coding-guidelines.md Outdated
Comment thread docs-wiki/Coding-guidelines.md Outdated
Comment thread docs-wiki/Coding-guidelines.md Outdated
Comment thread docs-wiki/Coding-guidelines.md Outdated
Comment thread docs-wiki/Coding-guidelines.md Outdated
@flanakin flanakin added this to the v15 milestone Aug 2, 2026
…-first rules

Fold the 'When contains is required' and 'Verify before you swap' H3s
into the main table, add in~/has_any rows, reframe the contains->has
row as a verified behavioral change, and back the =~ vs tolower()==
row with measured numbers (~5x CPU on a 29.7M-row Costs table).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extends the KQL section with a "Joins and lookups" subsection in the
same intent-first format as the string-matching rules: the innerunique
default trap, lookup as the canonical dimension-enrichment form,
mandatory dimension dedup (take_any by key, not distinct), leftanti for
exclusions, fullouter key coalescing, ARG constraints (no lookup, no
hints, 3-join limit), and the KqlJoinKinds.Tests.ps1 lint that enforces
explicit join kinds with a per-file ratchet baseline.

Grounded in the findings and live validation of PR #2225.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RolandKrummenacher RolandKrummenacher changed the title Add KQL string matching rules to the coding guidelines Add KQL string matching and join/lookup rules to the coding guidelines Aug 3, 2026
Probing every join flavor plus lookup against Azure Resource Graph
showed the ARG docs understate support in both directions: rightouter
and fullouter work (undocumented), while lookup, all semi/anti flavors,
and in/!in with a subquery are rejected with InvalidQuery. Since
leftanti does not exist in ARG, the leftouter + isempty emulation is
documented as the one acceptable exclusion form there (with a
key-unique right side), and the guideline now points to the
KqlJoinKinds.Tests.ps1 enforcement of the rejected-operator ban.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

docs-wiki/Coding-guidelines.md:113

  • The doc references src/powershell/Tests/Lint/KqlJoinKinds.Tests.ps1 / KqlJoinKinds.Tests.ps1, but that file does not exist in the repository. This makes the “enforced by …” guidance untraceable for contributors; either add the referenced test or update the text to point at the correct enforcement location (or remove the specific path).
> **Azure Resource Graph is not ADX.** ARG queries (workbooks, recommendation queries, the alerts logic app) support no `lookup`, no join hints, and have a documented limit of 3 joins per query. Live-probing every flavor (2026-08) gave a matrix that differs from the [ARG docs](https://learn.microsoft.com/azure/governance/resource-graph/concepts/query-language) in both directions: **supported** – `inner`, `innerunique`, `leftouter`, `rightouter`, `fullouter`; **rejected with `InvalidQuery`** – `lookup`, `leftsemi`, `leftanti`, `rightsemi`, `rightanti`, and `in`/`!in` with a subquery. Two consequences: the explicit-`kind` rule is the only defense against the `innerunique` default (verified live: a 40-row left side with one distinct key returns 1 row from a bare join, 40 from `kind=inner`), and **exclusions in ARG must use the `leftouter` + `where isempty(...)` emulation** – keep the right side key-unique (`distinct <key>` only) so it cannot fan out. `KqlJoinKinds.Tests.ps1` fails the build if an ARG surface uses `lookup` or a semi/anti flavor.

**Verify before you swap.** Converting `join` to `lookup` drops the duplicated key columns (`Key1`) from the output – confirm nothing downstream references them. Adding a dimension dedup changes which row wins for duplicate keys – confirm the surviving values are equivalent (or pick deterministically with `arg_max()`). For conversions on hub transforms, run the old and new pipeline over the same data and compare row counts *and* values, not just execution success.

These rules are enforced on every pull request by `src/powershell/Tests/Lint/KqlJoinKinds.Tests.ps1`: any `join` without an explicit `kind=` fails the build, across every KQL-carrying surface (hub scripts, query catalog, workbooks, recommendation queries, the alerts logic app, optimization engine, and published docs). Pre-existing bare joins are baselined per file as a ratchet – counts can only go down, and lowering the baseline is enforced when a file is cleaned up (#2228 tracks the backlog).

docs-wiki/Coding-guidelines.md:105

  • This join guidance uses escaped pipes (\|) inside inline code in the table. When someone copies from the raw markdown (or if the renderer doesn’t strip the escape), they’ll paste \| join ..., which is invalid KQL. Consider formatting these examples so the text is copy/paste-safe without backslashes (for example, avoid leading pipes in the snippet, or use a pipe representation that doesn’t require escapes in tables).
| `\| join (T) on Key` | `\| join kind=... (T) on Key` | Bare join = `innerunique`: left side deduplicated per key, rows silently dropped. State the intent, always |
| `\| join kind=leftouter (SmallDim) on Key` | `\| lookup kind=leftouter (SmallDim) on Key` | Fact-to-dimension enrichment is what `lookup` is for: broadcast, no duplicated key column. (ADX / Log Analytics only – ARG has no `lookup`) |
| `lookup (Dim \| distinct Key, Col1, Col2) on Key` | `lookup (Dim \| summarize take_any(Col1), take_any(Col2) by Key) on Key` | `distinct` over multiple columns still yields >1 row per key when the other columns differ – fact rows multiply |
| `join kind=leftouter (X) on K \| where isempty(K1)` | `join kind=leftanti (X) on K` | `leftanti` is duplicate-proof and never materializes right-side columns |
| `summarize Total=... \| join ... on 1 == 1` | `let Total = toscalar(...)` | `on 1 == 1` is not valid KQL (verified: fails with `General_BadRequest`); `toscalar()` also avoids a second full-table scan |

AGENTS.md:216

  • This bullet references src/powershell/Tests/Lint/KqlJoinKinds.Tests.ps1, but that file does not exist in the repository. Either add the referenced test or update the bullet to point to the actual enforcement location (or remove the specific path) so contributors can verify the rule.
- KQL joins: Never write a bare `| join` — always state `kind=` explicitly (the `innerunique` default deduplicates the left side and silently drops rows; enforced by `src/powershell/Tests/Lint/KqlJoinKinds.Tests.ps1`). Prefer `lookup` over `join` for enriching a fact table from a small dimension (ADX/Log Analytics only — not available in Azure Resource Graph), dedupe the dimension side with `summarize take_any(...) by <key>` (not `distinct`), and use `kind=leftanti` for exclusions instead of `kind=leftouter` + `where isempty(...)` — except in ARG, which rejects `lookup` and all semi/anti flavors (verified live), so exclusions there use the `leftouter` + `isempty()` emulation with a key-unique right side. See the "Joins and lookups" section of `docs-wiki/Coding-guidelines.md`

Review feedback (Copilot): the avoid/prefer table used backslash-escaped
pipes inside inline code, which renders fine but pastes as invalid KQL
from the raw markdown. The snippets are reworded to not need pipes at
all. The KqlJoinKinds.Tests.ps1 references now note the test is added
in PR #2225, since it does not exist on this branch; merge order is
called out in the PR description.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@RolandKrummenacher

Copy link
Copy Markdown
Collaborator Author

Addressed the three suppressed review comments in 3c658f4:

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs: Review 👀 PR that is ready to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants