Skip to content

[BUGFIX] QueryExpectation warns that a parameterized query has no data asset reference #12243

Description

@feiiiiii5

What happens

QueryExpectation.validate_configuration decides whether a query references the batch's data asset by splitting the query on five separators and looking for {batch} among the pieces (expectation.py#L1957-L1966):

parsed_query: Set[str] = {
    x for x in re.split(", |\(|\n|\)| |/", query) if x.upper() and ...
}
assert "{batch}" in parsed_query, (
    "Your query appears to not be parameterized for a data asset. ...")

The character that happens to sit next to the placeholder decides the answer, so a query that GE renders perfectly well warns that it has no data asset. The opposite also happens: a {batch} that appears only in a comment passes silently, which is the case the warning exists to catch.

Reproduce

import warnings
from great_expectations.expectations.expectation import QueryExpectation


class RowCount(QueryExpectation):
    metric_name = "probe.row_count"
    query = "SELECT COUNT(*) FROM {batch}"

    def _validate(self, metrics, runtime_configuration=None, execution_engine=None):
        return {"success": True, "result": {}}


for q in [
    "SELECT COUNT(*) FROM {batch}",          # control
    "SELECT COUNT(*) FROM {batch};",         # same query, trailing semicolon
    "SELECT\tCOUNT(*)\n\tFROM\t{batch}",     # same query, tab indented
    "SELECT COUNT(*) FROM my_table\n-- switch this to {batch}\n",  # placeholder in a comment only
]:
    with warnings.catch_warnings(record=True) as caught:
        warnings.simplefilter("always")
        RowCount(query=q).validate_configuration()
    print(repr(q), "->", [str(w.message)[:60] for w in caught if "data asset" in str(w.message)])

Measured on develop @ 8bfb2b2:

query warns "not be parameterized for a data asset" correct?
SELECT COUNT(*) FROM {batch} no yes
SELECT COUNT(*) FROM {batch}; yes no - it is parameterized
SELECT\tCOUNT(*)\n\tFROM\t{batch} yes no - it is parameterized
SELECT COUNT(*) FROM my_table\n-- switch this to {batch}\n no no - it is not parameterized

Why it is wrong

  1. The renderer is delimiter-blind. A QueryExpectation query is rendered with str.format (query_metric_provider.py#L192, query.format(batch=..., **parameters)), which substitutes {batch} regardless of what follows it, so the check is stricter than the code it is a precondition for. Semicolon-terminated queries are ordinary: they are what a user gets from a .sql file, and GE's own changelog ships examples such as "SELECT * FROM {batch} WHERE passenger_count > 6;". CRLF and tabs are what a checkout and an indented query give you.
  2. The sibling check for the same parameter already does it differently. UnexpectedRowsExpectation._validate_query asks string.Formatter().parse(query) for the format field names (unexpected_rows_expectation.py#L131-L132), which is delimiter-independent. On 8bfb2b2 the two classes disagree on the same string: for "SELECT * FROM {batch};" and for the tab-indented variant, UnexpectedRows stays silent while QueryExpectation warns.
  3. Comments and string literals are not code. A placeholder inside them is substituted but never names the data asset being read, so its presence should not suppress the warning - and its being space-delimited inside the comment is precisely what makes today's split accept it.

QueryExpectation is not subclassed anywhere in great_expectations/ (a grep of the package finds the name only in expectation.py), so this affects user-authored custom QueryExpectations - the ones the "how to create a custom QueryExpectation" guide tells people to write.

Expected

{batch} appearing in the query's code counts as a data asset reference whatever delimits it; {batch} appearing only inside a comment or a string literal does not count.

Notes on a fix

Blank the comment and string-literal regions in one left-to-right pass, then look for the placeholder across any run of whitespace or SQL punctuation. {{batch}} (which str.format renders as a literal {batch}, not a substitution) must keep warning, and the second check in the same try block - hard-coded references - is a separate question and should keep its current behaviour.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions