Fix escaping during check execution - #1486
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1486 +/- ##
==========================================
- Coverage 92.62% 92.44% -0.19%
==========================================
Files 141 141
Lines 13579 13593 +14
Branches 151 151
==========================================
- Hits 12578 12566 -12
- Misses 932 959 +27
+ Partials 69 68 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
✅ 960/960 passed, 49 skipped, 6h35m8s total Running from acceptance #5675 |
|
✅ 1/1 passed, 27m41s total Running from mcp #424 |
|
✅ 195/195 passed, 1 skipped, 7h4m5s total Running from anomaly #1789 |
There was a problem hiding this comment.
Pull request overview
This PR addresses #1481 by ensuring column names that require SQL identifier escaping (spaces, non-ASCII, etc.) are safely handled during check execution (not just validation), preventing INVALID_IDENTIFIER failures when building Spark expressions.
Changes:
- Add
utils.normalize_column_expr()and use it incheck_funcs._get_column_expr()so string column inputs are safely passed toF.expr. - Adjust
get_normalized_column_and_expr()so display/normalized names come from the original user input string (not the escaped Spark expression). - Extend unit + integration coverage, including contract-rule generation for escaped columns.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/databricks/labs/dqx/utils.py |
Introduces normalize_column_expr() and supporting regexes to decide when to back-quote identifiers vs pass through SQL expressions. |
src/databricks/labs/dqx/check_funcs.py |
Uses normalize_column_expr() when converting string inputs via F.expr, and tweaks name normalization/display behavior. |
src/databricks/labs/dqx/datacontract/contract_rules_generator.py |
Uses normalize_column_expr() when embedding column names into generated sql_expression rules. |
tests/unit/test_utils.py |
Adds parametric unit tests for normalize_column_expr() behavior (identifiers, nested paths, expressions). |
tests/unit/test_row_checks.py |
Adds unit test ensuring normalized/display names derive from the raw input column string. |
tests/unit/test_datacontract_generator.py |
Adds unit test verifying generated SQL expressions back-quote a column requiring escaping. |
tests/integration/test_apply_checks.py |
Adds integration tests proving row-level checks run end-to-end on columns requiring escaping (metadata + class-based APIs). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if SQL_EXPRESSION_CHAR_PATTERN.search(column): | ||
| return column | ||
|
|
| col_expr = _get_column_expr(column) | ||
| column_str = get_column_name_or_alias(col_expr) | ||
| col_str_norm = get_column_name_or_alias(col_expr, normalize=True) | ||
| if isinstance(column, str): |
There was a problem hiding this comment.
shouldn't this logic be implemented inside get_column_name_or_alias?
We already handle the case there:
if isinstance(column, str):
col_str = column
we just need to extend it with an option to normalize:
if isinstance(column, str):
if normalize:
col_str = normalize_col_str(column)
else:
col_str = column
Then we don't need to change check_funcs.py
column_str = get_column_name_or_alias(col_expr)
col_str_norm = get_column_name_or_alias(col_expr, normalize=True)
| Returns: | ||
| A string safe to pass to ``F.expr``. | ||
| """ | ||
| if SQL_EXPRESSION_CHAR_PATTERN.search(column): |
There was a problem hiding this comment.
Finding: word-only SQL expressions are back-quoted into a single identifier
normalize_column_expr back-quotes word-only SQL expressions (containing only letters/digits/spaces, no operator characters) as a single identifier, breaking expressions like col IS NOT NULL or a AND b when passed as a check column.
Failure scenario: A check func receives column="col IS NOT NULL" (a supported string expression). SQL_EXPRESSION_CHAR_PATTERN finds no operator char (space is not in the class), so the whole string is treated as a name and quoted to `col IS NOT NULL`. F.expr("col IS NOT NULL") then resolves it as one literal column that does not exist, whereas the pre-PR F.expr("col IS NOT NULL") parsed a boolean expression. Regression for operator-free expressions.
|
|
||
| Args: | ||
| column: Column reference provided as a string (plain name, nested path, or SQL expression). | ||
|
|
There was a problem hiding this comment.
Finding: column names containing expression characters are not escaped (incomplete fix)
normalize_column_expr does not escape column names that contain any expression character (hyphen, comma, parentheses, quote, etc.), so such names are passed through unquoted and misparsed — the escaping fix is incomplete.
Failure scenario: A DataFrame has a column literally named gross-margin or amount (usd). normalize_column_expr sees the - / ( as an expression character and returns the string unchanged, so F.expr("gross-margin") parses as subtraction (gross MINUS margin) and F.expr("amount (usd)") is a syntax error. These names require back-quoting, but the heuristic only handles spaces and non-ASCII letters, leaving a large class of names-requiring-escaping still broken.
| col_expr = _get_column_expr(column) | ||
| column_str = get_column_name_or_alias(col_expr) | ||
| col_str_norm = get_column_name_or_alias(col_expr, normalize=True) | ||
| if isinstance(column, str): |
There was a problem hiding this comment.
Finding: display/normalized names now derived from raw input, changing messages and check names
For string column inputs, the display name (col_expr_str) and normalized name (col_str_norm) now come from the raw input rather than the parsed Column, changing error messages and auto-generated check names for pre-quoted or aliased/expression strings.
Failure scenario: User passes column="Customer Name" (already back-quoted). Pre-PR column_str was parsed to Customer Name; now column_str = column, so the error message reads Column '`Customer Name`' value is null with literal backticks. Likewise column="a AS b" previously produced display/name from the alias b; now it is the raw a AS b (col_str_norm becomes a_as_b), changing user-visible messages and check names.
| segment | ||
| if VALID_UNQUOTED_IDENTIFIER_PATTERN.match(segment) or (segment.startswith("`") and segment.endswith("`")) | ||
| else quote_column_name(segment) | ||
| ) |
There was a problem hiding this comment.
Finding: unreachable dead-code branch
The segment.startswith("`") and segment.endswith("`") guard inside this comprehension is unreachable dead code.
Failure scenario: SQL_EXPRESSION_CHAR_PATTERN (line 82) includes the backtick character, so any string containing a backtick returns early at the top of the function. A segment can only contain a backtick if the whole string did, meaning the already-back-quoted check can never evaluate. It is dead code that suggests a mis-scoped guard and adds maintenance noise.
Changes
This PR adds escaping for column names during check execution.
Note: Auto-generated check names (e.g. when checks are not named by the user) are normalized to replace reserved characters with
_.Linked issues
Resolves #1481
Tests
Documentation and Demos