diff --git a/src/repl.rs b/src/repl.rs index 0eb31db..d3defc4 100644 --- a/src/repl.rs +++ b/src/repl.rs @@ -435,8 +435,8 @@ impl<'a> Repl<'a> { Err(err) => return Err(ReplError::SqlExecutor(err)), }; - // Print results - if let Some(result_set) = result { + // Print every statement's result set, each with its own header. + for result_set in &result { if result_set.rows.is_empty() { println!("Query returned no rows"); } else { @@ -445,11 +445,16 @@ impl<'a> Repl<'a> { println!("{}", result_set.columns.join(",")); // Print rows - for row in result_set.rows { + for row in &result_set.rows { println!("{}", row.join(",")); } } - } else if self.show_changes { + } + + // The change count is reported independently of whether the line also + // produced rows. Gating it on "no result set" hid the count for a + // trailing DML on a line like `SELECT ...; DELETE ...`. + if self.show_changes && self.executor.last_statement_changed_rows() { // For non-SELECT statements that don't return rows (INSERT, UPDATE, DELETE) // Try to display the number of affected rows if show_changes is enabled // Reported even when zero. With `.changes on` a statement that diff --git a/src/sql_executor.rs b/src/sql_executor.rs index 0dc8c69..83b7b15 100644 --- a/src/sql_executor.rs +++ b/src/sql_executor.rs @@ -40,6 +40,8 @@ pub struct SqlExecutor<'a> { /// Number of rows affected by the last DML statement affected_rows: usize, + /// Whether the last executed script contained a row-counting DML statement + dml_executed: bool, } impl<'a> SqlExecutor<'a> { @@ -55,6 +57,7 @@ impl<'a> SqlExecutor<'a> { modified_tables: HashSet::with_capacity(DEFAULT_TABLE_CAPACITY), config: config.clone(), affected_rows: 0, + dml_executed: false, } } @@ -87,6 +90,7 @@ impl<'a> SqlExecutor<'a> { // Track affected rows from the last statement self.affected_rows = result.affected_rows; + self.dml_executed = result.dml_executed; // Set delimiter on each result table to match config let mut tables = result.tables; @@ -193,16 +197,29 @@ impl<'a> SqlExecutor<'a> { /// * `sql` - SQL statement to execute /// /// # Returns - /// * `Result>` - Optional ResultSet containing query results - pub fn execute_sql(&mut self, sql: &str) -> Result> { + /// * One ResultSet per statement that produced rows, in order. + /// + /// Returning only the last one hid earlier result sets AND suppressed the + /// change count of a trailing DML statement, because the caller treated + /// "there is a result set" as "this line produced no changes". + pub fn execute_sql(&mut self, sql: &str) -> Result> { let result = self.execute(sql)?; - // The REPL shows one result set at a time; a multi-statement line - // reports the last statement's. - Ok(result.last().map(|table| ResultSet { - columns: table.columns().to_vec(), - rows: table.rows_as_strings(), - })) + Ok(result + .iter() + .map(|table| ResultSet { + columns: table.columns().to_vec(), + rows: table.rows_as_strings(), + }) + .collect()) + } + + /// 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 } /// Check if a table exists diff --git a/src/vm/ast_compat.rs b/src/vm/ast_compat.rs index eaa569c..c5a6b1f 100644 --- a/src/vm/ast_compat.rs +++ b/src/vm/ast_compat.rs @@ -168,7 +168,28 @@ pub(crate) fn create_table_options(options: &CreateTableOptions) -> &[SqlOption] /// understands (`delimiter`, `header`, ...). Non key-value forms yield `None`. pub(crate) fn sql_option_key_value(option: &SqlOption) -> Option<(String, String)> { match option { - SqlOption::KeyValue { key, value } => Some((key.value.clone(), value.to_string())), + SqlOption::KeyValue { key, value } => { + // Unwrap the literal rather than rendering it back to SQL. + // + // `value.to_string()` produces the SQL text INCLUDING quotes, so a + // double-quoted option value kept its quote characters: `WITH + // (delimiter = "|")` yielded the three-character delimiter `"|"` + // and wrote rows no reader could parse back. + let text = match value { + Expr::Value(v) => match &v.value { + sqlparser::ast::Value::SingleQuotedString(s) + | sqlparser::ast::Value::DoubleQuotedString(s) + | sqlparser::ast::Value::Number(s, _) => s.clone(), + other => other.to_string(), + }, + // A double-quoted option value parses as a QUOTED IDENTIFIER, + // not a string literal, so `WITH (delimiter = "|")` arrives + // here as an Ident. Its `value` is already unquoted. + Expr::Identifier(ident) => ident.value.clone(), + other => other.to_string(), + }; + Some((key.value.clone(), text)) + } _ => None, } } diff --git a/src/vm/compiler.rs b/src/vm/compiler.rs index 3490705..c7de94c 100644 --- a/src/vm/compiler.rs +++ b/src/vm/compiler.rs @@ -5,8 +5,9 @@ //! It implements a visitor pattern to walk the AST generated by sqlparser. use super::ast_compat::{ - create_table_options, delete_from, func_args, group_by_exprs, insert_target, is_group_by_all, - is_order_by_all, object_name_last, order_by_is_asc, query_limit, query_offset, query_order_by, + create_table_options, delete_from, func_args, func_is_distinct, group_by_exprs, insert_target, + is_group_by_all, is_order_by_all, object_name_last, order_by_is_asc, query_limit, query_offset, + query_order_by, }; use sqlparser::ast::{ BinaryOperator, CaseWhen, DataType as SqlDataType, Expr, Function, FunctionArg, @@ -18,7 +19,8 @@ use sqlparser::parser::Parser; use std::collections::HashMap; use super::bytecode::{ - Instruction, OpCode, Program, ResultSchema, AGG_AVG, AGG_COUNT, AGG_MAX, AGG_MIN, AGG_SUM, + Instruction, OpCode, Program, ResultSchema, AGG_AVG, AGG_COUNT, AGG_DISTINCT, AGG_MAX, AGG_MIN, + AGG_SUM, }; use crate::aggregate::AggregateFunction; use crate::capacity::DEFAULT_CURSOR_CAPACITY; @@ -467,6 +469,24 @@ impl<'a> SqlCompiler<'a> { } } + /// The AggStep/AggFinal function-type word for an aggregate CALL, + /// including its DISTINCT flag. + /// + /// Prefer this over `agg_func_type`, which sees only the name. Four sites + /// derived the type independently and two of them forgot to OR in + /// AGG_DISTINCT, so DISTINCT was silently dropped from every aggregate in + /// a grouped query -- and HAVING, which did set the flag, then failed to + /// match the aggregate it had been given. + pub(crate) fn agg_func_type_for(func: &Function) -> i64 { + let name = func.name.to_string().to_uppercase(); + Self::agg_func_type(&name) + | if func_is_distinct(func) { + AGG_DISTINCT + } else { + 0 + } + } + /// Convert aggregate function name to type code pub(crate) fn agg_func_type(name: &str) -> i64 { match name { @@ -1304,7 +1324,22 @@ impl<'a> SqlCompiler<'a> { let ctx = NameCtx::single(table, cursor_idx); for (i, ob) in order_by.iter().enumerate() { - self.code_expr(&ob.expr, &ctx, Some(start_reg + i as i64))?; + let dest = start_reg + i as i64; + // `ORDER BY 2` names the second output column, it is not the + // constant 2. + if let Some(out_idx) = Self::order_by_ordinal(&ob.expr, col_count)? { + let col_idx = columns[out_idx]; + self.emit( + OpCode::Column, + cursor_idx, + col_idx as i64, + dest, + None, + &format!("r[{}] = ORDER BY position {}", dest, out_idx + 1), + ); + } else { + self.code_expr(&ob.expr, &ctx, Some(dest))?; + } } for (i, col_idx) in columns.iter().enumerate() { @@ -4802,6 +4837,19 @@ impl<'a> SqlCompiler<'a> { cursor_idx: usize, target_reg: i64, ) -> SqawkResult<()> { + // A start position is required. + // + // sqlparser 0.62 parses `SUBSTR(x)` into this node with no start, and + // defaulting it to 1 returned the whole string unchanged -- so a + // dropped or mistyped argument silently produced untruncated output. + // The arity check in compile_func_substr became unreachable for this + // spelling, since SUBSTR no longer arrives as a Function call. + if substring_from.is_none() { + return Err(SqawkError::InvalidSqlQuery( + "SUBSTR requires at least two arguments".to_string(), + )); + } + let src_reg = self.allocate_register(); self.compile_where_operand(sub_expr, table, cursor_idx, src_reg)?; @@ -6625,13 +6673,18 @@ impl<'a> SqlCompiler<'a> { if !order_by.is_empty() { let mut keys: Vec = Vec::with_capacity(order_by.len()); for ob in order_by { - let pos = - Self::projection_position(&select.projection, &ob.expr).ok_or_else(|| { - SqawkError::InvalidSqlQuery(format!( - "ORDER BY expression must appear in the SELECT list: {}", - ob.expr - )) - })?; + let out_columns = self.program.result_schema.columns.len(); + let pos = match Self::order_by_ordinal(&ob.expr, out_columns)? { + Some(idx) => idx, + None => Self::projection_position(&select.projection, &ob.expr) + .or_else(|| self.result_column_position(&ob.expr)) + .ok_or_else(|| { + SqawkError::InvalidSqlQuery(format!( + "ORDER BY expression must appear in the SELECT list: {}", + ob.expr + )) + })?, + }; keys.push(format!( "{}:{}", pos, @@ -6694,6 +6747,69 @@ impl<'a> SqlCompiler<'a> { None } + /// A positional ORDER BY key (`ORDER BY 2`) as a zero-based output index. + /// + /// SQL numbers these from 1 against the SELECT list. Compiling such a key + /// as an ordinary expression yields a CONSTANT, giving every row the same + /// sort key and silently turning the sort into a no-op -- worse than the + /// old code, which at least rejected the form. + fn order_by_ordinal(expr: &Expr, out_columns: usize) -> SqawkResult> { + let n = match expr { + Expr::Value(ValueWithSpan { + value: Value::Number(n, _), + .. + }) => n, + _ => return Ok(None), + }; + let idx: usize = n.parse().map_err(|_| { + SqawkError::InvalidSqlQuery(format!("Invalid ORDER BY position: {}", n)) + })?; + if idx == 0 || idx > out_columns { + return Err(SqawkError::InvalidSqlQuery(format!( + "ORDER BY position {} is out of range (1..{})", + idx, out_columns + ))); + } + Ok(Some(idx - 1)) + } + + /// Position of `expr` among the RESULT columns, by name. + /// + /// The projection-list search cannot see through a wildcard: `SELECT *` + /// contributes one `SelectItem::Wildcard` while producing many output + /// columns, so ordering a `SELECT *` join by any column found nothing and + /// was rejected. The result schema is already built by the time + /// post-processing is emitted and names every output column, including + /// the expanded ones. + /// + /// Matches a qualified name exactly first, then falls back to the bare + /// column name, so `ORDER BY name` finds an output column called `c.name`. + fn result_column_position(&self, expr: &Expr) -> Option { + let want = match expr { + Expr::Identifier(ident) => ident.value.to_ascii_lowercase(), + Expr::CompoundIdentifier(parts) if parts.len() >= 2 => format!( + "{}.{}", + parts[parts.len() - 2].value.to_ascii_lowercase(), + parts[parts.len() - 1].value.to_ascii_lowercase() + ), + _ => return None, + }; + + let cols = &self.program.result_schema.columns; + if let Some(i) = cols + .iter() + .position(|c| c.name.to_ascii_lowercase() == want) + { + return Some(i); + } + // Unqualified key against qualified output columns, or vice versa. + let bare = want.rsplit('.').next().unwrap_or(&want).to_string(); + cols.iter().position(|c| { + let n = c.name.to_ascii_lowercase(); + n == bare || n.rsplit('.').next().map(|x| x == bare).unwrap_or(false) + }) + } + /// Evaluate a LIMIT/OFFSET operand, which must be a constant. fn const_i64(expr: &Expr, what: &str) -> SqawkResult { match expr { diff --git a/src/vm/compiler_aggregate.rs b/src/vm/compiler_aggregate.rs index 446f281..723d614 100644 --- a/src/vm/compiler_aggregate.rs +++ b/src/vm/compiler_aggregate.rs @@ -4,8 +4,8 @@ use sqlparser::ast::{Expr, FunctionArg, FunctionArgExpr, Select, SelectItem}; -use super::ast_compat::{func_args, func_is_distinct, group_by_exprs}; -use super::bytecode::{OpCode, ResultSchema, AGG_DISTINCT}; +use super::ast_compat::{func_args, group_by_exprs}; +use super::bytecode::{OpCode, ResultSchema}; use super::compiler::{NameCtx, SqlCompiler}; use crate::error::{SqawkError, SqawkResult}; use crate::table::Table; @@ -191,16 +191,9 @@ impl<'a> SqlCompiler<'a> { ) -> SqawkResult> { match expr { Expr::Function(func) => { - let name = func.name.to_string().to_uppercase(); - // DISTINCT rides along as a flag bit on the function type. - // It used to be dropped entirely -- func.distinct was never - // read -- so COUNT(DISTINCT department) counted rows. - let func_type = Self::agg_func_type(&name) - | if func_is_distinct(func) { - AGG_DISTINCT - } else { - 0 - }; + // DISTINCT rides along as a flag bit on the function type, + // derived in one place so no site can forget it. + let func_type = Self::agg_func_type_for(func); // Check for COUNT(*) if let Some(FunctionArg::Unnamed(FunctionArgExpr::Wildcard)) = @@ -387,7 +380,7 @@ impl<'a> SqlCompiler<'a> { if let Expr::Function(func) = expr { let name = func.name.to_string().to_uppercase(); if matches!(name.as_str(), "COUNT" | "SUM" | "AVG" | "MIN" | "MAX") { - let func_type = Self::agg_func_type(&name); + let func_type = Self::agg_func_type_for(func); if let Some(FunctionArg::Unnamed(FunctionArgExpr::Wildcard)) = func_args(func).first() { @@ -774,13 +767,7 @@ impl<'a> SqlCompiler<'a> { // SUM(salary), whichever appeared first, and filtered on // entirely the wrong column. MIN vs MAX bound correctly, which // is why it stayed hidden. - let name = func.name.to_string().to_uppercase(); - let func_type = Self::agg_func_type(&name) - | if func_is_distinct(func) { - AGG_DISTINCT - } else { - 0 - }; + let func_type = Self::agg_func_type_for(func); let wanted_col: Option = match func_args(func).first() { Some(FunctionArg::Unnamed(FunctionArgExpr::Wildcard)) => None, diff --git a/src/vm/compiler_ddl.rs b/src/vm/compiler_ddl.rs index 0679e6e..e57cade 100644 --- a/src/vm/compiler_ddl.rs +++ b/src/vm/compiler_ddl.rs @@ -432,8 +432,10 @@ impl<'a> SqlCompiler<'a> { let delimiter = with_options.iter().find_map(|opt| { let (key, value) = sql_option_key_value(opt)?; if key.to_lowercase() == "delimiter" { - // `value` is rendered SQL, so a string literal arrives quoted. - return Some(value.trim_matches('\'').to_string()); + // sql_option_key_value unwraps the literal, so no quote + // stripping is needed -- and stripping only single quotes left + // double-quoted values carrying their quotes. + return Some(value); } None }); diff --git a/src/vm/compiler_join.rs b/src/vm/compiler_join.rs index 04243b4..313ed2e 100644 --- a/src/vm/compiler_join.rs +++ b/src/vm/compiler_join.rs @@ -1117,7 +1117,7 @@ impl<'a> SqlCompiler<'a> { Expr::Function(func) => { let name = func.name.to_string().to_uppercase(); if matches!(name.as_str(), "COUNT" | "SUM" | "AVG" | "MIN" | "MAX") { - let func_type = Self::agg_func_type(&name); + let func_type = Self::agg_func_type_for(func); let col_ref = if let Some(FunctionArg::Unnamed(FunctionArgExpr::Wildcard)) = func_args(func).first() { @@ -2580,22 +2580,35 @@ impl<'a> SqlCompiler<'a> { .position(|r| r.eq_ignore_ascii_case(table_ref)) .ok_or_else(|| SqawkError::TableNotFound(table_ref.clone()))?; - // Find the column in that table - let col_idx = tables[tbl_idx] - .column_index(col_name) + let col_idx = Self::column_index_ci(tables[tbl_idx], col_name) .ok_or_else(|| SqawkError::ColumnNotFound(col_name.clone()))?; Ok((tbl_idx, col_idx)) } Expr::Identifier(ident) => { - // Unqualified column - search all tables + // Unqualified column: search every table, matching + // case-insensitively and reporting ambiguity. + // + // This used an exact-match lookup and returned the first table + // that had the column, while NameCtx (which serves the + // expression compiler) matches case-insensitively and treats a + // name present in two tables as an error. The same identifier + // therefore resolved differently depending on which resolver + // the code path reached. let col_name = &ident.value; + let mut found: Option<(usize, usize)> = None; for (tbl_idx, table) in tables.iter().enumerate() { - if let Some(col_idx) = table.column_index(col_name) { - return Ok((tbl_idx, col_idx)); + if let Some(col_idx) = Self::column_index_ci(table, col_name) { + if found.is_some() { + return Err(SqawkError::InvalidSqlQuery(format!( + "Column '{}' is ambiguous across the tables in FROM", + col_name + ))); + } + found = Some((tbl_idx, col_idx)); } } - Err(SqawkError::ColumnNotFound(col_name.clone())) + found.ok_or_else(|| SqawkError::ColumnNotFound(col_name.clone())) } _ => Err(SqawkError::UnsupportedSqlFeature(format!( "Unsupported column expression in multi-table query: {:?}", @@ -2604,6 +2617,15 @@ impl<'a> SqlCompiler<'a> { } } + /// Case-insensitive column lookup, matching how `NameCtx` resolves names. + fn column_index_ci(table: &Table, name: &str) -> Option { + let want = name.to_ascii_lowercase(); + table + .column_metadata() + .iter() + .position(|c| c.name.to_ascii_lowercase() == want) + } + /// Build result schema for a multi-table aggregate query fn build_multi_table_agg_schema( &self, diff --git a/src/vm/compiler_window.rs b/src/vm/compiler_window.rs index b4335a0..fdcce68 100644 --- a/src/vm/compiler_window.rs +++ b/src/vm/compiler_window.rs @@ -10,6 +10,19 @@ use super::compiler::SqlCompiler; use crate::error::{SqawkError, SqawkResult}; use crate::table::{DataType, Table}; +/// Whether a window function is one of the aggregates, as opposed to a +/// ranking or offset function. +/// +/// Defined once: this list was spelled out at two sites, so adding an +/// aggregate window function meant editing both, and missing the second left +/// running totals in the output with nothing to indicate it. +fn is_aggregate_window_fn(name: &str) -> bool { + matches!( + name.to_uppercase().as_str(), + "SUM" | "COUNT" | "AVG" | "MIN" | "MAX" + ) +} + impl<'a> SqlCompiler<'a> { pub(crate) fn compile_select_with_window( &mut self, @@ -22,6 +35,22 @@ impl<'a> SqlCompiler<'a> { self.add_comment("Window function query"); + // A wildcard in a window query is not expanded by this path: the + // projection analysis below counts one output column per SELECT item, + // so `SELECT *, SUM(x) OVER (...)` emitted two columns for a + // four-column table and lost the rest. Reject it rather than return a + // silently truncated row. + if select.projection.iter().any(|i| { + matches!( + i, + SelectItem::Wildcard(_) | SelectItem::QualifiedWildcard(..) + ) + }) { + return Err(SqawkError::UnsupportedSqlFeature( + "SELECT * cannot be combined with a window function; list the columns".into(), + )); + } + // Analyze the projection to find window functions and their specs let (window_funcs, mut base_columns) = self.analyze_window_projection(&select.projection, table)?; @@ -500,23 +529,20 @@ impl<'a> SqlCompiler<'a> { // With an ORDER BY the running total is the correct answer, so this is // emitted only when the window is unordered. if order_cols.is_empty() && !partition_cols.is_empty() { + // With wildcards rejected above, each projection item yields + // exactly one output column, so the SELECT-list index IS the + // result-column index that WindowFinalize needs. let agg_output_positions: Vec = select .projection .iter() .enumerate() - .filter(|(_, item)| { - matches!( - item, - SelectItem::UnnamedExpr(Expr::Function(f)) - | SelectItem::ExprWithAlias { - expr: Expr::Function(f), - .. - } if f.over.is_some() - && matches!( - f.name.to_string().to_uppercase().as_str(), - "SUM" | "COUNT" | "AVG" | "MIN" | "MAX" - ) - ) + .filter(|(_, item)| match item { + SelectItem::UnnamedExpr(Expr::Function(f)) + | SelectItem::ExprWithAlias { + expr: Expr::Function(f), + .. + } => f.over.is_some() && is_aggregate_window_fn(&f.name.to_string()), + _ => false, }) .map(|(i, _)| i) .collect(); diff --git a/src/vm/engine.rs b/src/vm/engine.rs index ea47271..8777a37 100644 --- a/src/vm/engine.rs +++ b/src/vm/engine.rs @@ -892,7 +892,11 @@ impl<'a> VmEngine<'a> { // Create a new table from P4 specification // P4 format: "table_name:col1:type1,col2:type2,...|filepath|delimiter" let spec = inst.p4.clone().unwrap_or_default(); - let parts: Vec<&str> = spec.split('|').collect(); + // splitn, not split: the delimiter is the LAST field and may + // itself be '|'. Splitting on every '|' turned + // `WITH (delimiter = '|')` into an extra empty field and left + // the table with the default comma instead. + let parts: Vec<&str> = spec.splitn(3, '|').collect(); if parts.is_empty() { return Err(SqawkError::VmError( diff --git a/src/vm/mod.rs b/src/vm/mod.rs index 75966e7..249fe3a 100644 --- a/src/vm/mod.rs +++ b/src/vm/mod.rs @@ -47,6 +47,12 @@ pub struct VmExecutionResult { pub modified_tables: HashSet, /// Number of rows affected by the last DML statement (INSERT, UPDATE, DELETE) pub affected_rows: usize, + /// Whether any statement in this script was a row-counting DML. + /// + /// Distinguishes "a DML ran and changed nothing" from "no DML ran at all", + /// which a count of zero cannot express. Without it the REPL either hides + /// a genuine zero or prints a meaningless one after every SELECT. + pub dml_executed: bool, } /// Execute SQL using the VM execution engine @@ -111,6 +117,7 @@ pub fn execute_vm( let mut tables: Vec = Vec::new(); let mut modified_tables = HashSet::with_capacity(DEFAULT_TABLE_CAPACITY); let mut affected_rows: usize = 0; + let mut dml_executed = false; for statement in &statements { // PHASE 0: MATERIALIZE DERIVED TABLES. @@ -123,28 +130,45 @@ pub fn execute_vm( let mut statement = statement.clone(); let derived = materialize_derived_tables(&mut statement, database, verbose)?; - // PHASE 1: SQL -> BYTECODE, against the CURRENT database state. - let program = { - let mut compiler = compiler::SqlCompiler::new(database, verbose); - compiler.compile_statement_program(&statement)? - }; + // Phases 1-3 run inside a closure so a failure anywhere still drops + // the derived tables registered above. Leaving them behind made the + // alias unusable for the rest of the session: a failed + // `SELECT bad FROM (SELECT ...) x` left `x` registered, so the next + // statement using the same alias failed with "shadows an existing + // table", and `.tables` listed a phantom table. + let outcome = (|| -> SqawkResult<(Option
, AppliedModifications)> { + // PHASE 1: SQL -> BYTECODE, against the CURRENT database state. + let program = { + let mut compiler = compiler::SqlCompiler::new(database, verbose); + compiler.compile_statement_program(&statement)? + }; - if verbose { - println!("Generated bytecode:"); - println!("{}", program); - } + if verbose { + println!("Generated bytecode:"); + println!("{}", program); + } - // PHASE 2: EXECUTE - let mut vm = engine::VmEngine::new_mut(database, verbose); - vm.init(program); - vm.execute()?; + // PHASE 2: EXECUTE + let mut vm = engine::VmEngine::new_mut(database, verbose); + vm.init(program); + vm.execute()?; - let result_table = vm.create_result_table()?; - let modifications = vm.take_modifications(); - drop(vm); + let result_table = vm.create_result_table()?; + let modifications = vm.take_modifications(); + drop(vm); - // PHASE 3: APPLY MODIFICATIONS before the next statement compiles. - let applied = apply_modifications(database, modifications, verbose)?; + // PHASE 3: APPLY MODIFICATIONS before the next statement compiles. + let applied = apply_modifications(database, modifications, verbose)?; + Ok((result_table, applied)) + })(); + + // Derived tables live only for the statement that declared them, and + // must be dropped whether or not it succeeded. + for name in &derived { + database.remove_table(name); + } + + let (result_table, applied) = outcome?; modified_tables.extend(applied.modified_tables); // A DML statement reports its own count, INCLUDING zero. @@ -159,11 +183,7 @@ pub fn execute_vm( // Which statements count is the same set SQL's `changes()` uses. if is_row_counting_dml(&statement) { affected_rows = applied.affected_rows; - } - - // Derived tables live only for the statement that declared them. - for name in derived { - database.remove_table(&name); + dml_executed = true; } if let Some(t) = result_table { @@ -175,6 +195,7 @@ pub fn execute_vm( tables, modified_tables, affected_rows, + dml_executed, }) } @@ -279,7 +300,17 @@ fn is_row_counting_dml(statement: &sqlparser::ast::Statement) -> bool { use sqlparser::ast::Statement; matches!( statement, - Statement::Insert(_) | Statement::Update(_) | Statement::Delete(_) + Statement::Insert(_) + | Statement::Update(_) + | Statement::Delete(_) + // TRUNCATE removes rows and apply_modifications counts them, so + // omitting it discarded a real count and reported the PREVIOUS + // statement's -- or, with the REPL now printing unconditionally, + // a confident "0 rows affected" for a table it had just emptied. + | Statement::Truncate(_) + // CREATE TABLE AS SELECT inserts rows, and those inserts are + // counted the same way. + | Statement::CreateTable(_) ) } diff --git a/tests/data/repeats_grouped.csv b/tests/data/repeats_grouped.csv new file mode 100644 index 0000000..df3bdf8 --- /dev/null +++ b/tests/data/repeats_grouped.csv @@ -0,0 +1,7 @@ +grp,val +a,1 +a,1 +a,2 +b,1 +b,1 +b,3 diff --git a/tests/data/windowed.csv b/tests/data/windowed.csv new file mode 100644 index 0000000..f2c73aa --- /dev/null +++ b/tests/data/windowed.csv @@ -0,0 +1,4 @@ +a,b,d,x +1,p,E,10 +2,q,E,20 +3,r,S,30 diff --git a/tests/golden/mod.rs b/tests/golden/mod.rs index 966f757..1ebd818 100644 --- a/tests/golden/mod.rs +++ b/tests/golden/mod.rs @@ -2289,3 +2289,137 @@ fn implicit_join_order_by_without_limit() -> Result<(), Box Result<(), Box> { + // Two sites derived the aggregate function type independently and only one + // OR-ed in the DISTINCT flag, so DISTINCT was dropped whenever GROUP BY + // was present -- while the same aggregate without GROUP BY was correct. + assert_query( + &["tests/data/repeats_grouped.csv"], + "SELECT grp, COUNT(DISTINCT val), SUM(DISTINCT val) FROM repeats_grouped GROUP BY grp", + "grp,COUNT,SUM\na,2,3\nb,2,4", + ) +} + +#[test] +fn distinct_aggregate_in_having() -> Result<(), Box> { + // HAVING did set the flag, so it could not match the aggregate the + // projection had registered without it. + assert_query( + &["tests/data/repeats_grouped.csv"], + "SELECT grp, COUNT(DISTINCT val) FROM repeats_grouped GROUP BY grp \ + HAVING COUNT(DISTINCT val) > 1", + "grp,COUNT\na,2\nb,2", + ) +} + +#[test] +fn order_by_over_select_star_join() -> Result<(), Box> { + // The ORDER BY key was matched against SELECT-list text, which cannot see + // through a wildcard, so this was rejected outright. + assert_query( + &["tests/data/customers.csv", "tests/data/purchases.csv"], + "SELECT * FROM customers, purchases WHERE customers.id = purchases.customer_id \ + ORDER BY purchases.id DESC", + "customers.id,customers.name,purchases.id,purchases.customer_id,purchases.item\n\ + 2,Ben,12,2,Desk\n\ + 1,Ann,11,1,Pen\n\ + 1,Ann,10,1,Book", + ) +} + +#[test] +fn order_by_bare_key_against_qualified_projection() -> Result<(), Box> { + assert_query( + &["tests/data/users.csv", "tests/data/orders.csv"], + "SELECT users.name, orders.id FROM users JOIN orders ON users.id = orders.user_id \ + ORDER BY id DESC LIMIT 2", + "users.name,orders.id\nJohn,105\nJane,104", + ) +} + +#[test] +fn positional_order_by_sorts() -> Result<(), Box> { + // `ORDER BY 2` compiled to the constant 2, giving every row the same sort + // key and silently turning the sort into a no-op. + assert_query( + &["tests/data/customers.csv"], + "SELECT name, id FROM customers ORDER BY 2 DESC", + "name,id\nCara,3\nBen,2\nAnn,1", + ) +} + +#[test] +fn positional_order_by_out_of_range_is_rejected() -> Result<(), Box> { + assert_query_fails( + &["tests/data/customers.csv"], + "SELECT name FROM customers ORDER BY 5", + "out of range", + ) +} + +#[test] +fn window_aggregate_over_explicit_columns() -> Result<(), Box> { + assert_query( + &["tests/data/windowed.csv"], + "SELECT a, d, SUM(x) OVER (PARTITION BY d) FROM windowed", + "a,d,SUM\n1,E,30\n2,E,30\n3,S,30", + ) +} + +#[test] +fn window_with_wildcard_is_rejected() -> Result<(), Box> { + // The window path counts one output column per SELECT item, so a wildcard + // silently emitted a truncated row (2 columns for a 4-column table). + assert_query_fails( + &["tests/data/windowed.csv"], + "SELECT *, SUM(x) OVER (PARTITION BY d) FROM windowed", + "SELECT * cannot be combined with a window function", + ) +} + +#[test] +fn substr_without_start_is_rejected() -> Result<(), Box> { + // sqlparser parses SUBSTR(x) into Expr::Substring with no start, and + // defaulting it to 1 returned the whole string unchanged. + assert_query_fails( + &["tests/data/employees.csv"], + "SELECT SUBSTR(name) FROM employees", + "SUBSTR requires at least two arguments", + ) +} + +#[test] +fn unqualified_ambiguous_column_in_multi_table_is_rejected( +) -> Result<(), Box> { + // `id` exists in both tables. The multi-table resolver returned the first + // match where NameCtx reports ambiguity, so the same name resolved + // differently depending on which resolver the path reached. + assert_query_fails( + &["tests/data/users.csv", "tests/data/orders.csv"], + "SELECT id, COUNT(*) FROM users, orders WHERE users.id = orders.user_id GROUP BY id", + "ambiguous", + ) +} + +#[test] +fn unqualified_column_in_multi_table_is_case_insensitive() -> Result<(), Box> +{ + // The same resolver matched exactly while NameCtx matched + // case-insensitively, so `NAME` resolved in one path and not the other. + assert_query( + &["tests/data/users.csv", "tests/data/orders.csv"], + "SELECT NAME, COUNT(*) FROM users, orders WHERE users.id = orders.user_id GROUP BY NAME", + "NAME,COUNT\nJane,2\nJohn,3", + ) +} diff --git a/tests/repl/commands.rs b/tests/repl/commands.rs index 65f4a97..cde042e 100644 --- a/tests/repl/commands.rs +++ b/tests/repl/commands.rs @@ -206,3 +206,100 @@ fn test_repl_zero_row_update_reports_zero() { "second UPDATE matched nothing and must report 0, got:\n{stdout}" ); } + +/// A trailing DML statement on a multi-statement REPL line must still report +/// its change count, and a plain SELECT must not report one at all. +/// +/// execute_sql returned only the last row-producing statement, so a trailing +/// DELETE was invisible and repl.rs never reached the change-count branch. +/// Reporting the count unconditionally then printed "0 rows affected" after +/// every SELECT, so the executor now tracks whether a DML actually ran. +#[test] +fn test_repl_reports_changes_for_trailing_dml() { + use std::io::Write; + use std::process::{Command, Stdio}; + + let dir = crate::helpers::create_temp_dir().expect("temp dir"); + let dest = dir.path().join("people.csv"); + std::fs::copy("tests/data/people.csv", &dest).expect("copy fixture"); + + let mut child = Command::new(env!("CARGO_BIN_EXE_sqawk")) + .arg("-i") + .arg(format!("people={}", dest.to_str().unwrap())) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("failed to start sqawk"); + + child + .stdin + .as_mut() + .expect("stdin") + .write_all( + b".changes on\n\ + SELECT * FROM people; DELETE FROM people WHERE id = 1;\n\ + SELECT id FROM people;\n\ + .exit\n", + ) + .expect("write"); + + let out = child.wait_with_output().expect("wait"); + let stdout = String::from_utf8_lossy(&out.stdout); + let counts: Vec<&str> = stdout + .lines() + .filter(|l| l.ends_with("rows affected")) + .collect(); + + assert_eq!( + counts, + ["1 rows affected"], + "the DELETE must report once and the SELECTs not at all, got:\n{stdout}" + ); + assert!( + stdout.contains("Query returned 3 rows") && stdout.contains("Query returned 2 rows"), + "both SELECT result sets should print, got:\n{stdout}" + ); +} + +/// A statement that fails must not leave its derived-table alias registered. +#[test] +fn test_repl_failed_derived_table_does_not_leak() { + use std::io::Write; + use std::process::{Command, Stdio}; + + let mut child = Command::new(env!("CARGO_BIN_EXE_sqawk")) + .arg("-i") + .arg("tests/data/people.csv") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("failed to start sqawk"); + + child + .stdin + .as_mut() + .expect("stdin") + .write_all( + b"SELECT nosuch FROM (SELECT * FROM people) x;\n\ + SELECT id FROM (SELECT * FROM people) x;\n\ + .tables\n\ + .exit\n", + ) + .expect("write"); + + let out = child.wait_with_output().expect("wait"); + let stdout = String::from_utf8_lossy(&out.stdout); + let stderr = String::from_utf8_lossy(&out.stderr); + let combined = format!("{stdout}{stderr}"); + + assert!( + !combined.contains("shadows an existing table"), + "the failed statement leaked its alias, got:\n{combined}" + ); + assert!( + !stdout.lines().any(|l| l.trim() == "x"), + ".tables lists a phantom derived-table alias, got:\n{stdout}" + ); +}