Skip to content

Fix validation for columns requiring SQL identifier escaping#1342

Open
ghanse wants to merge 5 commits into
mainfrom
fix_escaped_identifiers
Open

Fix validation for columns requiring SQL identifier escaping#1342
ghanse wants to merge 5 commits into
mainfrom
fix_escaped_identifiers

Conversation

@ghanse

@ghanse ghanse commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Changes

This PR adds a fallback path to validate bare column names that require SQL identifier escaping (e.g. Customer Name).

Because checks can pass columns as string names or Spark SQL expressions, a 2-pass validation is performed:

  1. Try the input string with pyspark.sql.functions.expr(...)
  2. If step 1 fails, escape the input string with backticks, then retry with pyspark.sql.functions.expr(...)

Linked issues

Resolves #1334

Tests

  • manually tested
  • added unit tests
  • added integration tests
  • added end-to-end tests
  • added performance tests

Documentation and Demos

  • added/updated demos
  • added/updated docs
  • added/updated agent skills

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.77%. Comparing base (ab30bde) to head (9c17af7).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1342      +/-   ##
==========================================
+ Coverage   91.55%   92.77%   +1.22%     
==========================================
  Files         102      102              
  Lines       10429    10571     +142     
==========================================
+ Hits         9548     9807     +259     
+ Misses        881      764     -117     
Flag Coverage Δ
anomaly 53.78% <21.05%> (+32.87%) ⬆️
anomaly-serverless 53.79% <21.05%> (+32.86%) ⬆️
integration 50.64% <78.94%> (+0.32%) ⬆️
integration-serverless 51.01% <78.94%> (+0.26%) ⬆️
unit 59.19% <78.94%> (+0.35%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

✅ 827/827 passed, 42 skipped, 5h43m12s total

Running from acceptance #5254

@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

✅ 194/194 passed, 2 skipped, 7h15m37s total

Running from anomaly #1368

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

This pull request addresses schema-validation false negatives for DataFrames whose column names require Spark SQL identifier escaping (e.g., spaces or non-ASCII characters), ensuring has_valid_schema is not incorrectly skipped for valid columns (resolves #1334).

Changes:

  • Add a fallback validation path that retries failed string column references as backtick-quoted identifiers.
  • Improve skipped-check messaging to back-quote invalid column names that require SQL identifier escaping.
  • Add integration test coverage for special-character column names and for skipped-check suppression behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/databricks/labs/dqx/utils.py Adds quote_column_name() helper for backtick-quoting/escaping SQL identifiers.
src/databricks/labs/dqx/manager.py Implements 2-pass column validation (raw F.expr, then quoted identifier) and adjusts invalid-column display for skip messaging.
tests/integration/test_apply_checks.py Adds an integration test ensuring has_valid_schema runs with space/non-ASCII column names.
tests/integration/test_apply_checks_suppress_skipped.py Adds integration tests for missing special-character columns, including suppress_skipped behavior and expected skip messages.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/databricks/labs/dqx/manager.py
Comment thread tests/integration/test_apply_checks.py Outdated
Comment thread src/databricks/labs/dqx/manager.py

@mwojtyczka mwojtyczka 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.

Summary

This PR correctly fixes validation for column names that require SQL identifier escaping (e.g. Customer Name, Ääkkönen) by adding a 2-pass validation in _is_invalid_column and back-quoting names in skip/error messages. The approach is sound and the integration coverage is good.

Correctness — Verified ParseException subclasses AnalysisException, so raw strings that fail parsing are caught and routed to the fallback. The fallback (_is_invalid_quoted_column) only returns valid when the backtick-quoted form resolves to a real column, so genuine junk and expressions are still handled correctly. No false negatives found.

Tests — Integration tests cover the real fix well (valid special-char columns run instead of being skipped, missing ones still skipped/suppressed, message correctly back-quoted). One gap: quote_column_name is a pure, workspace-free helper with no unit test — see inline comment.

Minor / optional — Several existing call sites hand-roll backtick quoting (checks_storage.py:454, anomaly/anomaly_llm_explainer.py:228, llm/llm_pk_detector.py:172, datacontract/contract_rules_generator.py:512). Now that quote_column_name exists, converging those on the shared helper would be a nice follow-up (out of scope here).

Comment thread src/databricks/labs/dqx/utils.py
@mwojtyczka mwojtyczka added the under-review This PR is currently being reviewed by one of DQX maintainers. label Jul 22, 2026

@mwojtyczka mwojtyczka 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.

missing test, otherwise LGTM

@mwojtyczka mwojtyczka 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.

LGTM

@mwojtyczka mwojtyczka added Approved to Merge When PR is reviewed and approved. To be merged once all tests pass and removed under-review This PR is currently being reviewed by one of DQX maintainers. labels Jul 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ 1/1 passed, 17m53s total

Running from mcp #75

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

Labels

Approved to Merge When PR is reviewed and approved. To be merged once all tests pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: has_valid_schema incorrectly marks columns with spaces or non-ASCII characters as invalid

3 participants