Fix all ten defects from the high-effort branch review

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

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<Option<ResultSet>>` - Optional ResultSet containing query results
-    pub fn execute_sql(&mut self, sql: &str) -> Result<Option<ResultSet>> {
+    /// * 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<Vec<ResultSet>> {
         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

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,
     }
 }

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<String> = 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<Option<usize>> {
+        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<usize> {
+        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<i64> {
         match expr {

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<Option<(i64, i64)>> {
         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<usize> = match func_args(func).first() {
                     Some(FunctionArg::Unnamed(FunctionArgExpr::Wildcard)) => None,

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
         });