fix(sigma): handle unary negation in sql_to_sigma_formula reverse translation - #433
mattsenicksigma wants to merge 1 commit into
Conversation
Closes apache#426. `_render_sql_node` had no branch for `exp.Neg`, causing `sql_to_sigma_formula` to return `None` for any negated SQL expression (e.g. `-"X"`, `-1`) even though the forward path emits `exp.Neg` correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| return f"({_render_sql_node(node.this, dataset_alias)} AND {_render_sql_node(node.expression, dataset_alias)})" | ||
| if isinstance(node, exp.Or): | ||
| return f"({_render_sql_node(node.this, dataset_alias)} OR {_render_sql_node(node.expression, dataset_alias)})" | ||
| if isinstance(node, exp.Neg): |
There was a problem hiding this comment.
I think this regresses two input classes from a safe failure into an unsafe one.
The bar -{inner} assumes every child renders self-delimited. Two reachable children don't: DPipe/Concat is the one binary operator branch that doesn't self parenthesize, and Not emits a bare NOT (...).
Today I believe it degrades visibly: the column drops and an EXPRESSION_NOT_TRANSLATABLE issue is logged. After this change it produces a formula our own parse_formula, and per _resolve_formula docstring the data model API validates the whole document before applying, so that fails the entire create/update upload rather than one column.
I suggest to use:
if isinstance(node, exp.Neg):
inner = _render_sql_node(node.this, dataset_alias)
if isinstance(node.this, (exp.Column, exp.Literal)):
return f"-{inner}"
return f"-({inner})"The redundant parens on cases like -SUM([X]) -> (SUM([X])) are harmless.
Also the PR description justification ("binary operator nodes already self-parenthesize in _render_sql_node) isn't accurate for the Concat branch, which is what makes the bare prefix unsafe here. Maybe worth correcting so the next reviewer doesn't rely on it 😄
| ("SUM(ss_ext_sales_price)", "store_sales", "Sum([ss_ext_sales_price])"), | ||
| ("COUNT(DISTINCT customer_id)", "customer", "CountDistinct([customer_id])"), | ||
| ("CASE WHEN status = 'won' THEN 1 ELSE 0 END", "deals", 'If((["status"] = "won"), 1, 0)'.replace('["status"]', "[status]")), | ||
| ('-"X"', "T", "-[X]"), |
There was a problem hiding this comment.
This is related to the other comment.
These assert string equality on the two simplest inputs, so nothing here would crash. I have two proposals:
- Add
assert parse_formula(result) is not Nonetotest_reverse_translation_basic, that alone catches the-(NOT "A")regression, and several pre-existing emit-an-invalid-formula bugs. - Add the compound cases:
('-(NOT "A")', "T", "-(NOT ([A]))"),
('-("A" || "B")', "T", "-([A] & [B])"),
('-("A" + "B")', "T", "-(([A] + [B]))"),
('-(-"A")', "T", "-(-[A])"),
Nit: ('-"X"', "T", "-[X]") passes dataset_alias="T" that the case never exercises, since the column is unqualified, '-"T","X"' would.
Summary
Fixes #426.
_render_sql_nodehad no branch forexp.Neg, sosql_to_sigma_formulareturnedNonefor any negated SQL expression — even though the forward path (-[X]→-"X") correctly emitsexp.Neg.exp.Negbranch to_render_sql_nodeinsigma_formula.py-"X"→-[X]and-1→-1No extra parentheses are needed: binary operator nodes already self-parenthesize in
_render_sql_node(e.g.Add→([A] + [B])), so a bare-prefix handles all cases correctly.Test plan
uv run pytest tests/test_sigma_formula.py— 53/53 passed including the two new negation cases🤖 Generated with Claude Code