fix(databricks): match cascade-drop references case-insensitively - #428
christianeu-db wants to merge 2 commits into
Conversation
|
fyi @Haoranli503, this is the the python counterpart to your Java fix here: Haoranli503@a991e68 |
Databricks SQL identifiers are case-insensitive, but the cascade-drop match in `_references_dropped` was case-sensitive. A metric such as `COUNT(DISTINCT REGION_NAME)` therefore failed to match a dropped `region_name` and survived as a dangling reference in the emitted Metric View. Add `re.IGNORECASE` to both patterns in `_references_dropped` (the `measure(<name>)` pattern and the bare-dimension token pattern). The word-boundary guards are unchanged, so qualified `alias.col` references are still not over-dropped. Adds the reported repro as a regression test that fails without the fix. Fixes apache#422 Co-authored-by: Isaac <no-reply@databricks.com>
6491970 to
502d736
Compare
| # column sharing a dropped name is not falsely cascade-dropped. | ||
| if d != self_name and re.search( | ||
| r"(?<![\w.])" + re.escape(d) + r"(?![\w.])", expr): | ||
| r"(?<![\w.])" + re.escape(d) + r"(?![\w.])", expr, re.IGNORECASE): |
There was a problem hiding this comment.
Making this match case insensitive means a dropped field bare name now collides with any surviving expression that happens to contain the same token in a different case, including common SQL keywords/function names (COUNT, SUM, MIN, MAX, AVG, DATE, ...).
Since these are pretty common as both column names and SQL functions, this looks like a regression to me more than a narrow fix.
Maybe worth to tightening the match (e.g. exclude known SQL keywords, or restrict to idenfiier-shaped tokens that are not immediately followed by ().
There was a problem hiding this comment.
Good catch - you're right that matching bare tokens case-insensitively widens a collision. Digging in, it's actually a pre-existing issue that IGNORECASE broadened rather than a brand-new one: even before this PR a dropped dimension named exactly COUNT already cascade-dropped a surviving COUNT(...) measure, because the bare-token match never distinguished a column reference from a function call. Case-insensitivity just extends that to case variants.
As shown in the valid snippet of DuckDB SQL below, some vendors support mixed-case keywords:
WITH q as (select 1 as SUM)
SELECT sUM(SUM) from qI went with the (?!\s*() exclusion rather than a SQL-keyword blocklist deliberately: a blocklist would re-open #422 for any identifier that collided with a keyword; a dimension literally named date/count would then no longer cascade-drop and would survive as a dangling reference.
One caveat a bare keyword that is not a function call (a type in CAST(x AS DATE), a unit in EXTRACT(YEAR FROM d), CASE ... END, DISTINCT), or text inside a string literal, can still be over-dropped. Fully resolving column-vs-keyword needs a full parse, which is the ambiguity the function documents; I kept this fix to the function-call case you flagged but opted not to extend the scope of the PR, especially as the python interpreter will be sunset in favor of the Java interpreter, hopefully soon.
| # word char or `.` excludes both substrings of a larger identifier and | ||
| # qualified paths (`alias.name` / `name.col`), so a join alias or joined | ||
| # column sharing a dropped name is not falsely cascade-dropped. | ||
| if d != self_name and re.search( |
There was a problem hiding this comment.
This self reference guard is still case-sensitive here, but the regex it protects is case-insensitive (see below): so it no longer reliably excludes self-references when names differ only in case.
Should we d.lower() != self_name.lower() (or compare against a case-folded self_name) to match the new case-sensitive semantics?
There was a problem hiding this comment.
Agreed - the guard should be case-folded to match the regex. Changed to d.lower() != self_name.lower(), and mirrored the same case-folded self-guard. Added a regression test with a dropped REGION coexisting with a surviving region self-reference.
Address review feedback on apache#428: - Exclude function-call tokens (`NAME(...)`) from the bare-dimension match, so a dropped dim named like a SQL function (e.g. `count`) does not falsely cascade-drop a surviving `COUNT(...)`. This also fixes the pre-existing exact-case collision. - Case-fold the self-reference guard on both the dimension and `measure(<name>)` branches, so a column is never dropped for referencing itself under a different case. - Add regression tests for both (each fails without its fix). Co-authored-by: Isaac <no-reply@databricks.com>
|
@jbonofre thank you kindly for the review. I've addressed the comments and added a few regression tests. I've flagged some cases that likely require a fully parser but wanted to hold off for a separate issue / PR to avoid scope creep. cc @Haoranli503 for the Java converters |
|
@christianeu-db thank you sooooo much for your updates! I'm doing a new pass 😄 |
2 participants
Summary
When a field is dropped (e.g. no DATABRICKS/ANSI_SQL dialect), the Databricks
converter cascade-drops any metric/dimension that references it, so the output
never carries a dangling reference. The reference match in
_references_droppedwas case-sensitive, but Databricks SQL identifiers are case-insensitive. A metric
like
COUNT(DISTINCT REGION_NAME)therefore failed to match a droppedregion_nameand survived as a dangling reference in the emitted Metric View.The fix adds
re.IGNORECASEto both patterns in_references_dropped(themeasure(<name>)pattern and the bare-dimension token pattern). The word-boundaryguards are unchanged, so qualified
alias.colreferences are still notover-dropped.
Related Issues
Fixes #422
Checklist
Converters
converters/is updated to reflect spec or ontology changesTests
pytest/ CI green)New regression test
test_cascade_drop_matches_dropped_name_case_insensitivelyreproduces the reported case (a
T_SQL-onlyregion_namedropped, plus aDATABRICKS metric
region_count = COUNT(DISTINCT REGION_NAME)). It fails withoutthe fix (the measure survives) and passes with it. The full
converters/databrickssuite passes (96 tests).Compliance
This pull request and its description were written by Isaac.