[BUGFIX] query asset SQL ending in a comment fails validation - #12124
Open
nanjeshramesh wants to merge 1 commit into
Open
[BUGFIX] query asset SQL ending in a comment fails validation#12124nanjeshramesh wants to merge 1 commit into
nanjeshramesh wants to merge 1 commit into
Conversation
…dation A query asset whose SQL ends in a line comment (like "-- note") failed with a database syntax error on every SQL backend. GX wraps the raw query text in a subquery before running metrics against it, compiling it as "(<text>) AS anon_1" on one line, so the appended ")" and alias landed inside the comment and the statement never closed. The issue pointed at three call sites in SqlAlchemyExecutionEngine that do this wrap via .columns().subquery(). Those are real and now share one helper, but they weren't the whole story: _subselectable builds the same kind of subquery a different way, through sa.select(sa.text(...)).subquery(), which isn't a TextClause and so never went through the fix at those three sites. That method turns out to be the one SQLite actually hits for query assets (its partition clause is never sa.true(), so it always ends up there), which is why the original repro failed specifically on SQLite. Both paths now run the raw text through a small helper that appends a newline before wrapping if the text doesn't already end in one, so a trailing comment can't reach the closing paren and alias. Checked the neighboring cases too: a comment in the middle of the query and an already closed /* */ comment at the end both still work fine, same as before. An unterminated block comment at the end is a different problem that a newline can't solve on its own, closing it properly would mean actual comment parsing, so I left that alone and called it out in the PR instead of trying to solve it here.
👷 Deploy request for niobium-lead-7998 pending review.Visit the deploys page to approve it
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #12122.
Turned out there are two separate places doing the wrap, not the
three called out in the issue.
get_domain_records, resolve_metric_bundle, and _count_query_parameters
each independently wrap a raw TextClause into a subquery via
.columns().subquery(). Those are the three sites named in the issue,
now sharing one helper (_wrap_raw_sql_as_subquery).
_subselectable does effectively the same thing but through
sa.select(sa.text(...)).subquery() instead, since it works with a
plain string, not a TextClause, so it never triggered the isinstance
check the other three sites use. This is the path SQLite takes for
query assets specifically: its partition clause is sa.text("1 = 1"),
never sa.true(), so the early return that skips this method never
fires. That's why the repro in the issue failed on SQLite the way it
did.
Both paths now go through _ensure_sql_text_ends_with_newline before
wrapping, so a comment that runs to the end of the text can't reach
whatever gets appended after it.
What I checked:
(ran it locally). Compiles correctly against the PostgreSQL dialect
too, though I don't have a local Postgres to actually run it
against.
either way.
this already worked.
newline can't fix this one, since nothing after an unclosed block
comment is ever "safe" no matter how many lines you add. Actually
closing it would need real comment-state parsing, which felt like
more than this fix should take on. Flagging it in case it's worth
its own issue.
New test file:
tests/integration/data_sources_and_expectations/data_sources/test_query_asset_sql_comments.py,
covering the first three cases above against both SQLite and
Postgres.