Conversation
Every finding traced to the same shape: a path that had not yet been unified drifted from the one that had, and the two now disagreed. Silent wrong answers: - DISTINCT was dropped from every aggregate in a GROUP BY query. Four sites derived the aggregate function type independently and two forgot to OR in AGG_DISTINCT, so `COUNT(DISTINCT val) ... GROUP BY grp` counted rows while the same aggregate without GROUP BY was correct. It also broke HAVING, which did set the flag and so could not match what the projection had registered. All four now go through one agg_func_type_for(func). - Positional ORDER BY compiled to a constant, giving every row the same sort key and silently making the sort a no-op. `ORDER BY 2` now selects the second output column, and an out-of-range position is an error. - A wildcard in a window query emitted a truncated row: the projection analysis counts one output column per SELECT item, so `SELECT *, SUM(x) OVER (...)` returned two columns for a four-column table and WindowFinalize wrote the partition total over a data column. Rejected until wildcards are expanded there. The duplicated aggregate-window-name list is now defined once. - SUBSTR(x) with no start position returned the whole string unchanged. sqlparser 0.62 parses it as Expr::Substring, so the arity check in compile_func_substr had become unreachable for that spelling. - A WITH delimiter kept its quote characters, because the option value was rendered back to SQL rather than unwrapped: `delimiter = "|"` yielded the three-character delimiter `"|"`. Double-quoted values parse as quoted IDENTIFIERS, not string literals, and are now unwrapped as such. Fixing that exposed a second defect underneath: the CreateTable P4 spec is '|'-separated and was split on every '|', so a '|' delimiter was discarded and silently became a comma. Now splitn(3). Behavioural regressions introduced earlier in this branch: - ORDER BY over a SELECT * join was rejected outright, because keys were matched against SELECT-list text which cannot see through a wildcard. On main these queries returned rows (unsorted, ORDER BY ignored). Keys now fall back to the result schema, which names every output column including expanded ones, so they sort instead of erroring. - A failed statement left its materialized derived-table alias registered, so the alias was unusable for the rest of the session and `.tables` listed a phantom entry. Cleanup now runs whether or not the statement succeeded. - TRUNCATE reported "0 rows affected" for a table it had just emptied: is_row_counting_dml omitted it, and the REPL had started printing counts unconditionally. TRUNCATE and CREATE TABLE AS SELECT now count. - A trailing DML on a multi-statement REPL line was invisible: execute_sql returned only the last row-producing statement, and the REPL treated "there is a result set" as "no changes". It now returns every result set and reports the count separately, gated on whether a DML actually ran -- which also stops a plain SELECT printing a meaningless "0 rows affected". Consistency: - resolve_multi_table_column matched column names exactly and returned the first table having an unqualified column, where NameCtx matches case-insensitively and reports ambiguity. The same identifier resolved differently depending on which resolver the path reached. Now aligned. Thirteen regression tests, one per defect plus the REPL behaviours. 580 passing, clippy clean, fmt clean, release green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a set of correctness defects in Sqawk’s VM compilation/execution pipeline and REPL behavior that previously produced silent wrong answers or misleading output. The PR also adds regression tests (golden + REPL) and small supporting fixtures to lock in the corrected behavior.
Changes:
- Unifies aggregate DISTINCT flag handling and fixes ORDER BY resolution (including positional ORDER BY) to prevent silent no-op sorting and dropped DISTINCT under GROUP BY/HAVING.
- Hardens window-function compilation (reject wildcard projections) and fixes SUBSTR arity handling for
Expr::Substring. - Improves REPL/multi-statement execution reporting (multiple result sets + correct “rows affected” printing) and ensures derived-table aliases are always cleaned up on statement failure.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/repl/commands.rs | Adds REPL regression tests for multi-statement change counts and derived-table alias cleanup. |
| tests/golden/mod.rs | Adds golden regression tests covering DISTINCT-in-GROUP BY, ORDER BY fixes, window wildcard rejection, SUBSTR arity, and multi-table resolution behavior. |
| tests/data/windowed.csv | Adds fixture data for window-function regression tests. |
| tests/data/repeats_grouped.csv | Adds fixture data for DISTINCT aggregate regression tests. |
| src/vm/mod.rs | Ensures derived tables are removed even on failure; tracks whether any row-counting DML ran. |
| src/vm/engine.rs | Fixes CREATE TABLE spec parsing to support ` |
| src/vm/compiler.rs | Centralizes DISTINCT-aware aggregate function typing; fixes positional ORDER BY; improves ORDER BY resolution against result schema; rejects SUBSTR(x) with missing start. |
| src/vm/compiler_window.rs | Rejects SELECT * with window functions; deduplicates aggregate-window-function detection. |
| src/vm/compiler_join.rs | Uses DISTINCT-aware aggregate typing; aligns multi-table column resolution with NameCtx (case-insensitive + ambiguity detection). |
| src/vm/compiler_ddl.rs | Stops incorrectly stripping only single quotes from WITH (delimiter=...) values. |
| src/vm/compiler_aggregate.rs | Switches all aggregate typing to the centralized DISTINCT-aware helper. |
| src/vm/ast_compat.rs | Unwraps option literals/idents instead of re-rendering SQL (fixes quoted delimiter handling). |
| src/sql_executor.rs | Returns all result sets from multi-statement execution and exposes whether the script ran row-counting DML. |
| src/repl.rs | Prints all result sets from a line; prints row counts independently when applicable. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+311
to
314
| // CREATE TABLE AS SELECT inserts rows, and those inserts are | ||
| // counted the same way. | ||
| | Statement::CreateTable(_) | ||
| ) |
Comment on lines
+217
to
223
| /// Whether the last executed script ran a row-counting DML statement. | ||
| /// | ||
| /// Distinguishes "a DML ran and changed nothing" from "no DML ran", which | ||
| /// a count of zero cannot express on its own. | ||
| pub fn last_statement_changed_rows(&self) -> bool { | ||
| self.dml_executed | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every finding traced to the same shape: a path that had not yet been unified drifted from the one that had, and the two now disagreed.
Silent wrong answers:
DISTINCT was dropped from every aggregate in a GROUP BY query. Four sites derived the aggregate function type independently and two forgot to OR in AGG_DISTINCT, so
COUNT(DISTINCT val) ... GROUP BY grpcounted rows while the same aggregate without GROUP BY was correct. It also broke HAVING, which did set the flag and so could not match what the projection had registered. All four now go through one agg_func_type_for(func).Positional ORDER BY compiled to a constant, giving every row the same sort key and silently making the sort a no-op.
ORDER BY 2now selects the second output column, and an out-of-range position is an error.A wildcard in a window query emitted a truncated row: the projection analysis counts one output column per SELECT item, so
SELECT *, SUM(x) OVER (...)returned two columns for a four-column table and WindowFinalize wrote the partition total over a data column. Rejected until wildcards are expanded there. The duplicated aggregate-window-name list is now defined once.SUBSTR(x) with no start position returned the whole string unchanged. sqlparser 0.62 parses it as Expr::Substring, so the arity check in compile_func_substr had become unreachable for that spelling.
A WITH delimiter kept its quote characters, because the option value was rendered back to SQL rather than unwrapped:
delimiter = "|"yielded the three-character delimiter"|". Double-quoted values parse as quoted IDENTIFIERS, not string literals, and are now unwrapped as such. Fixing that exposed a second defect underneath: the CreateTable P4 spec is '|'-separated and was split on every '|', so a '|' delimiter was discarded and silently became a comma. Now splitn(3).Behavioural regressions introduced earlier in this branch:
ORDER BY over a SELECT * join was rejected outright, because keys were matched against SELECT-list text which cannot see through a wildcard. On main these queries returned rows (unsorted, ORDER BY ignored). Keys now fall back to the result schema, which names every output column including expanded ones, so they sort instead of erroring.
A failed statement left its materialized derived-table alias registered, so the alias was unusable for the rest of the session and
.tableslisted a phantom entry. Cleanup now runs whether or not the statement succeeded.TRUNCATE reported "0 rows affected" for a table it had just emptied: is_row_counting_dml omitted it, and the REPL had started printing counts unconditionally. TRUNCATE and CREATE TABLE AS SELECT now count.
A trailing DML on a multi-statement REPL line was invisible: execute_sql returned only the last row-producing statement, and the REPL treated "there is a result set" as "no changes". It now returns every result set and reports the count separately, gated on whether a DML actually ran -- which also stops a plain SELECT printing a meaningless "0 rows affected".
Consistency:
Thirteen regression tests, one per defect plus the REPL behaviours.
580 passing, clippy clean, fmt clean, release green.