fix(mssql,azuresql): keep OpenMetadata's own queries out of usage and lineage - #31163
fix(mssql,azuresql): keep OpenMetadata's own queries out of usage and lineage#31163IceS2 wants to merge 3 commits into
Conversation
… lineage SQL Server reported the connector's own reflection SQL back as user queries. Two independent defects had to be fixed together; either one alone leaves the behaviour unchanged. sys.query_store_query_text stores one row per statement, and a leading comment belongs to the batch rather than to any statement, so the header was discarded before it could ever be matched. Sending it after the first token keeps it inside the statement. This is the same reason Vertica already has an override, though there the history table drops the comment outright. The plan-cache path did keep the header, but sp_executesql puts the parameter declarations in front of it, so the position-0 anchor in the four usage, lineage and stored-procedure filters missed every parameterised statement. The patterns now match the header anywhere in the text. Azure SQL reuses MssqlUsageSource and so shares those queries, but its schema never declared supportsQueryComment, which left create_generic_db_connection skipping header injection for it entirely. Declaring the field turns tagging on. Verified against SQL Server 2022 on both pytds and pyodbc: the header now survives into Query Store and the plan cache for bare, dedented, CTE and parameterised statements, and none of them reach the parser.
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| stripped = statement.lstrip() | ||
| first_token = FIRST_TOKEN.match(stripped) | ||
| if not first_token or stripped.startswith("/*"): | ||
| return statement | ||
| leading_whitespace = statement[: len(statement) - len(stripped)] | ||
| token = first_token.group(0) | ||
| header = render_query_header(_pkg_version("openmetadata-ingestion")) | ||
| return f"{leading_whitespace}{token} {header}{stripped[len(token) :]}" |
There was a problem hiding this comment.
💡 Edge Case: Inline header injected inside a leading line comment (--)
inject_inline_query_header only skips statements whose first non-whitespace is a block comment (stripped.startswith("/*")). If a statement begins with a -- line comment (e.g. -- note SELECT ...), the first token is -- and the header is placed right after it: -- /* {...} */ note SELECT ..., so the header sits inside the line comment and is dropped — the exact failure this PR fixes. This is unlikely for OpenMetadata's own reflection queries today, so it is a latent gap rather than a live defect; if it matters, also guard stripped.startswith("--") (returning the statement unchanged) or inject after the first real keyword.
Leave statements starting with a line comment untouched, matching the existing block-comment guard.:
stripped = statement.lstrip()
first_token = FIRST_TOKEN.match(stripped)
if not first_token or stripped.startswith(("/*", "--")):
return statement
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
✅ TypeScript Types Auto-UpdatedThe generated TypeScript types have been automatically updated based on JSON schema changes in this PR. |
|
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsUpdates MSSQL and Azure SQL connectors to correctly inject and match query header comments, preventing internal reflection queries from polluting usage and lineage data. Consider ensuring that inline header injection safely handles statements starting with leading line comments. 💡 Edge Case: Inline header injected inside a leading line comment (--)📄 ingestion/src/metadata/ingestion/connections/headers.py:73-80 inject_inline_query_header only skips statements whose first non-whitespace is a block comment ( Leave statements starting with a line comment untouched, matching the existing block-comment guard.🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Jest test CoverageUI tests summary
|
|
🔴 Playwright Results — workflow failedValidated commit ✅ 670 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky Pipeline and setup failures (1)
PerformanceBlocking targets: ❌ unmet · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 1h 5m 56s ⏱️ Max setup 2m 58s · max shard execution 18m 59s · max shard-job elapsed before upload 32m 52s · reporting 7s 🌐 211.76 requests/attempt · 2.66 app boots/UI scenario · 5.88% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
SQL Server reports the connector's own reflection SQL back as user queries. There are two
independent defects behind it, one per code path, and fixing either alone changes nothing.
Query Store never sees the header
sys.query_store_query_textstores one row per statement, and a leading comment belongs tothe batch rather than to any statement, so SQL Server discards it:
No pattern can match a string that was never stored. Sending the header after the first token
keeps it inside the statement. This is why Vertica already has an override — though there the
history table drops the comment outright, which is a different failure with the same remedy.
Unlike Vertica's
statement.split(" "), the SQL Server version splits on the firstnon-whitespace token: 17 of the MSSQL query constants are
textwrap.dedentstrings that openwith a newline, and for those the naive split lands the comment between two whitespace runs —
still ahead of the first keyword, still discarded, and indistinguishable from success.
The plan cache keeps the header, but not at position 0
sp_executesqlputs the parameter declarations in front of the statement:The four usage, lineage and stored-procedure filters anchored the header at position 0, so they
missed every parameterised statement while working correctly for unparameterised ones. The
patterns now match the header anywhere in the text.
Azure SQL was never tagging anything
AzuresqlUsageSourceextendsMssqlUsageSourceand shares these queries, butazureSQLConnection.jsonnever declaredsupportsQueryComment, so thehasattr(connection, "supportsQueryComment")gate increate_generic_db_connectionskippedheader injection entirely. It advertised
supportsUsageExtractionwhile being unable toidentify its own queries. Declaring the field turns tagging on.
Verification
Against SQL Server 2022 on both pytds and pyodbc, the header now survives into Query Store and
the plan cache for bare, dedented, CTE and parameterised statements, and none of them reach the
parser. The integration test asserts this through the server rather than by inspecting strings —
a header at
"\n /* ... */ SELECT"looks correct, parses fine, and is still discarded.Metadata, usage, profiler and auto-classification pipelines were run end to end against a local
SQL Server for both connectors.
Cost
The unanchored predicate roughly doubles the cost of that one query — 338 ms to 662 ms measured
over 16 089 distinct query texts with a window covering all of them. It runs once per usage run
per database. No index is affected:
query_sql_textanddm_exec_sql_text.textarenvarchar(max), which SQL Server cannot use as an index key.Not covered
The dialect's own bootstrap queries (
fn_listextendedproperty,sys.system_views, theisolation-level probe) run on the raw DBAPI connection, outside
before_cursor_execute, so noinjection strategy can reach them. Excluding those needs a system-object deny-list, which is a
separate change. Query Store rows recorded before this fix stay untagged and age out with
retention.