Rifqi Hafizuddin Claude Opus 4.8 commited on
Commit
8f736f9
·
1 Parent(s): 08a0a4a

[NOTICKET] fix: compiler parity — F-17 fabricated column, F-18 LIKE nulls, F-25 empty in/not_in

Browse files

Three execution-verified findings from CODE_REVIEW_2026-07-23 that had no DEV_PLAN
row — the tracker went from #39 straight to #40 and lost them. Same IR must not mean
two different things depending on whether the source is a database or a file.

F-17 (High) — the bare-select check was gated on `if ir.group_by`, so a select mixing
a bare column with an aggregate and group_by=[] passed validation. Postgres then
rejects it loudly, but the pandas path silently DROPS the column while
`output_columns` (built from the select list, not the result) still advertises it.
`data_access` maps rows by name, so the missing name becomes None: the user gets a
real-looking table whose first column is entirely em-dash, presented as data. Wrong
answers presented as correct is the worst outcome for this product.

The check now fires whenever any aggregate is present. No false positives are
possible — a mixed select with no group_by is invalid SQL in every dialect — and the
planner's retry loop self-corrects it, exactly as the original Q1 rule does.

F-18 — `.astype(str)` ran before `na=False` could apply, so NaN/None became the
literal strings "nan"/"None" and matched any pattern containing those letters.
`region LIKE '%an%'` over a column with 200 nulls returned 200 extra rows versus SQL,
with nothing to flag it. Now only non-null values are coerced; null positions stay
False. Uses reindex rather than positional assignment to avoid pandas' incompatible-
dtype FutureWarning.

F-25 — `SqlCompiler` raised on an empty `in`/`not_in` list, while the pandas compiler
AND `_column_values`' own docstring implement the empty-set semantics. A two-step
plan whose first step legitimately returned zero rows ("which customers never
ordered?") hard-failed the task, skipped its dependents, tripped CK1, and answered
with an honest-failure message instead of the correct answer. Empty `in` now compiles
to `1 = 0` and empty `not_in` to `1 = 1` (dialect-portable, parses cleanly through
the sqlglot guard). A non-list value still raises — emptiness became meaningful, a
wrong type did not.

Verification: 16 new parity tests, all green. One existing test replaced —
`test_filter_in_empty_list_raises` pinned exactly the F-25 behaviour being changed;
it becomes two tests for the new semantics plus one keeping the non-list rejection.
Suite 424 passed / 0 failed / 7 skipped (was 394/0/7; +28 new, +2 net from the
replacement). Ruff on touched paths unchanged from HEAD. `import main` OK.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

src/query/compiler/pandas.py CHANGED
@@ -182,7 +182,18 @@ def _apply_filters(
182
  elif op == "is_not_null":
183
  mask &= series.notna()
184
  elif op == "like":
185
- mask &= series.astype(str).str.fullmatch(_like_to_regex(val), case=True, na=False)
 
 
 
 
 
 
 
 
 
 
 
186
  elif op == "between":
187
  mask &= (series >= val[0]) & (series <= val[1])
188
  return df[mask].copy()
 
182
  elif op == "is_not_null":
183
  mask &= series.notna()
184
  elif op == "like":
185
+ # In SQL, `NULL LIKE '<pattern>'` evaluates to NULL, so the row is
186
+ # excluded. `.astype(str)` used to run FIRST, converting NaN/None into the
187
+ # literal strings "nan"/"None" — by the time `na=False` would have applied
188
+ # there were no NAs left, so a pattern like '%an%' matched every null row.
189
+ # Coerce only the non-null values and leave null positions False. (F-18)
190
+ non_null = series[series.notna()]
191
+ matched = non_null.astype(str).str.fullmatch(
192
+ _like_to_regex(val), case=True, na=False
193
+ )
194
+ # Reindex rather than assign into a bool Series — a positional assignment
195
+ # trips pandas' incompatible-dtype FutureWarning when the subset is empty.
196
+ mask &= matched.reindex(series.index, fill_value=False).astype(bool)
197
  elif op == "between":
198
  mask &= (series >= val[0]) & (series <= val[1])
199
  return df[mask].copy()
src/query/compiler/sql.py CHANGED
@@ -229,10 +229,21 @@ class SqlCompiler(BaseCompiler):
229
  return f"{col_ref} IS NOT NULL"
230
 
231
  if op in _LIST_OPS:
232
- if not isinstance(f.value, list) or not f.value:
233
  raise SqlCompilerError(
234
- f"filters[{index}]: op {op!r} requires a non-empty list value"
235
  )
 
 
 
 
 
 
 
 
 
 
 
236
  placeholders = [
237
  ":" + self._next_param(params, param_seq, v) for v in f.value
238
  ]
 
229
  return f"{col_ref} IS NOT NULL"
230
 
231
  if op in _LIST_OPS:
232
+ if not isinstance(f.value, list):
233
  raise SqlCompilerError(
234
+ f"filters[{index}]: op {op!r} requires a list value"
235
  )
236
+ if not f.value:
237
+ # Empty reference set. `in []` matches nothing; `not_in []` matches
238
+ # everything — the correct set semantics, what `_column_values`'
239
+ # docstring already promises, and what the pandas compiler already
240
+ # does (`series.isin([])`). This used to raise, hard-failing the task
241
+ # and skipping its dependents: a legitimate two-step plan whose first
242
+ # step returned zero rows ("which customers never ordered?") produced
243
+ # an honest-failure message instead of the correct answer. Emitted as
244
+ # `1 = 0` / `1 = 1` rather than FALSE/TRUE so it stays dialect-portable
245
+ # and parses cleanly through the sqlglot guard. (F-25)
246
+ return "1 = 0" if op == "in" else "1 = 1"
247
  placeholders = [
248
  ":" + self._next_param(params, param_seq, v) for v in f.value
249
  ]
src/query/ir/validator.py CHANGED
@@ -87,18 +87,39 @@ class IRValidator:
87
  for i, col_id in enumerate(ir.group_by):
88
  self._require_column(columns_by_id, col_id, f"group_by[{i}]")
89
 
90
- # A grouped query must not select bare columns that aren't in group_by
91
- # the database rejects it only at execution ("must appear in the GROUP BY
92
- # clause"), which is past the planner's corrective-retry window. Catching
93
- # it here turns a failed turn into a self-correcting re-prompt.
94
- if ir.group_by:
 
 
 
 
 
 
 
 
 
 
 
95
  grouped = set(ir.group_by)
96
  for i, item in enumerate(ir.select):
97
  if item.kind == "column" and item.column_id not in grouped:
 
 
 
 
 
 
 
 
 
 
 
98
  raise IRValidationError(
99
  f"select[{i}].column_id {item.column_id!r} is selected bare "
100
- "while group_by is present every selected column must "
101
- "either appear in group_by or be wrapped in an aggregate "
102
  f'(e.g. {{"kind": "agg", "fn": "sum", '
103
  f'"column_id": {item.column_id!r}}})'
104
  )
 
87
  for i, col_id in enumerate(ir.group_by):
88
  self._require_column(columns_by_id, col_id, f"group_by[{i}]")
89
 
90
+ # A query mixing bare columns with aggregates must not select a bare column
91
+ # that isn't in group_by — the database rejects it only at execution ("must
92
+ # appear in the GROUP BY clause"), which is past the planner's corrective-retry
93
+ # window. Catching it here turns a failed turn into a self-correcting re-prompt.
94
+ #
95
+ # Extended 2026-07-23 (F-17): the check used to be gated on `if ir.group_by`,
96
+ # so a mixed select with group_by=[] passed. On a DB source Postgres then failed
97
+ # loudly, but on a TABULAR source the pandas compiler silently DROPPED the bare
98
+ # column while `output_columns` (built from the select list, not the result)
99
+ # still advertised it — the user got a real-looking table whose first column was
100
+ # entirely `—`. Wrong answers presented as correct is the worst outcome for this
101
+ # product, so the check now fires whenever ANY aggregate is present. No false
102
+ # positives are possible: a mixed select with no group_by is invalid SQL in
103
+ # every dialect.
104
+ has_agg = any(item.kind == "agg" for item in ir.select)
105
+ if ir.group_by or has_agg:
106
  grouped = set(ir.group_by)
107
  for i, item in enumerate(ir.select):
108
  if item.kind == "column" and item.column_id not in grouped:
109
+ context = (
110
+ "while group_by is present"
111
+ if ir.group_by
112
+ else "alongside an aggregate with no group_by"
113
+ )
114
+ fix = (
115
+ "every selected column must either appear in group_by or be "
116
+ "wrapped in an aggregate"
117
+ if ir.group_by
118
+ else "either add it to group_by or wrap it in an aggregate"
119
+ )
120
  raise IRValidationError(
121
  f"select[{i}].column_id {item.column_id!r} is selected bare "
122
+ f"{context}{fix} "
 
123
  f'(e.g. {{"kind": "agg", "fn": "sum", '
124
  f'"column_id": {item.column_id!r}}})'
125
  )