From b79ff88b4cf53dcfd98150e606b4063fc7f58d9d Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Tue, 1 Sep 2026 19:58:40 +0100 Subject: [PATCH] change the enhanced tui so it derives the schema from the data table directly rather than stripping out just names and then guessing. --- docs/TUI_FEATURES.md | 133 ++++++++++---- src/main.rs | 12 +- src/sql/cursor_aware_parser.rs | 188 +++++--------------- src/sql/hybrid_parser.rs | 6 + src/sql/parser/legacy.rs | 279 +++++++++++++++++++++++++----- src/sql/parser/mod.rs | 4 +- src/sql/recursive_parser.rs | 4 +- src/ui/state/state_coordinator.rs | 44 +++-- tests/completion_schema.rs | 134 ++++++++++++++ tests/datetime_completion.rs | 97 ++++++++++- tests/main.rs | 3 + 11 files changed, 666 insertions(+), 238 deletions(-) create mode 100644 tests/completion_schema.rs diff --git a/docs/TUI_FEATURES.md b/docs/TUI_FEATURES.md index 1d6dbc13..57b353f9 100644 --- a/docs/TUI_FEATURES.md +++ b/docs/TUI_FEATURES.md @@ -53,14 +53,17 @@ dividing line is whether a correct engine would still leave the user annoyed. ## Where this effort is up to -**Phase: opening.** T1 is the first entry and the first fix. It surfaced T2–T5 -in the course of being done, which is the expected pattern — the completer's -real problem is that it has no model of the data, and every feature worth having -is downstream of fixing that. - -**Recommended order: T2 → T3 → T4 → T5.** T2 is groundwork that is mostly -deletion and pays for itself immediately on any non-trading dataset. T3 is -mechanical but wants doing *before* T4, not as a retrofit. +**Phase: groundwork done.** T1 fixed the text half of completion — which span +gets replaced. T2 fixed the data half — what the completer knows about the +columns it is completing. Between them the completer now has both primitives +the remaining entries need: a byte span to splice over, and a typed schema. + +**Recommended order: T3 → T4 → T5**, with **T7** droppable anywhere — it is +independent of the others and mostly deletion. T3 is mechanical but wants doing +*before* T4, not as a retrofit. T4 is the first entry that consumes what T2 +captured (`ColumnInfo::cardinality`, `TableInfo::row_count`); those numbers are +already flowing and pinned by tests, so the gate can be designed against real +values rather than guessed at. --- @@ -106,38 +109,60 @@ mechanical but wants doing *before* T4, not as a retrofit. unit tests for the scanner. ### T2 — The completer has no schema, only column names -- **Status:** 🔴 OPEN — **the blocker; do this first** -- **Where:** `src/sql/parser/legacy.rs:119` (`Schema` / `TableInfo`), - `src/sql/cursor_aware_parser.rs:772` (`get_property_type`), - `src/ui/state/state_coordinator.rs:61` (`update_parser_with_refs`) -- **Observed:** `TableInfo` is `{ name: String, columns: Vec }` — names +- **Status:** 🟢 DONE 2026-09-01 +- **Where:** `src/sql/parser/legacy.rs` (`ColumnType`, `ColumnInfo`, `TableInfo`, + `Schema`), `src/sql/cursor_aware_parser.rs` (`get_property_type`), + `src/ui/state/state_coordinator.rs` (`schema_snapshot`) +- **Observed:** `TableInfo` was `{ name: String, columns: Vec }` — names only. Two consequences: - - `get_property_type()`, which decides string-methods vs `DateTime(`, is a + - `get_property_type()`, which decides string-methods vs `DateTime(`, was a **hardcoded list of trade-desk column names** (`platformorderid`, `counterparty`, `tradedate`, …) with `else => "string"`. For any other - dataset *every* column falls through to that else. A numeric column gets - offered `Contains('')`; a date column not on the list never gets + dataset *every* column fell through to that else. A numeric column got + offered `Contains('')`; a date column not on the list never got `DateTime(`. - - `Schema::new()` **defaults to the trade_deal schema**, so before a file - loads the completer suggests trading columns. -- **Impact:** every type-driven decision in the completer is wrong by default on - non-trading data. This is almost certainly a bigger day-to-day annoyance than - T3–T5 combined, and it blocks all of them. -- **The fix is mostly deletion.** `DataColumn` (`src/data/datatable.rs:69`) - already carries what is needed — `data_type: DataType`, `unique_values: - Option`, `null_count`, `nullable` — and `infer_column_types()` - populates all of it on every load path. `update_parser_with_refs` already runs - at the right moment holding the `DataView`; it just discards everything and - passes `Vec`. Give `TableInfo` a real `ColumnInfo { name, data_type, - cardinality, nullable }`, populate it there, delete the hardcoded list and the - trade_deal default. -- **Keep the boundary:** the schema should hold a **bounded snapshot**, never a - live handle to the `DataTable`. The parser being a pure function of - `(query, cursor, schema)` is what makes T1's tests cheap to write; handing it - live data gives that up. + - `Schema::new()` **defaulted to the trade_deal schema**, so before a file + loaded the completer suggested trading columns. +- **Impact:** every type-driven decision in the completer was wrong by default + on non-trading data, and it blocked T3–T5. +- **Fixed by:** `ColumnInfo { name, data_type, cardinality, nullable }` and + `TableInfo { name, columns, row_count }`. `ColumnInfo::from_data_column` + reads what `infer_column_types()` had already computed and thrown away on + every load path; `StateCoordinator::schema_snapshot` takes it at the three + points that previously passed `Vec`. `get_property_type` is now a + schema lookup, and the name list, the trade_deal default, and a third + dead backward scanner (`detect_method_call_context`, the same class of bug + T1 removed two of) are all deleted. `ColumnType` is deliberately coarser + than `DataType` — `Integer` vs `Float` changes no suggestion — and boolean + columns, which had no representation at all before, now offer `true`/`false` + after a comparison operator. +- **The boundary held:** the schema is a bounded snapshot, not a handle to the + `DataView`, so the parser stays a pure function of `(query, cursor, schema)` + and every test below runs without a terminal. Columns are snapshotted from + the *source* table rather than the view, so hiding a column in the TUI does + not make it uncompletable. +- **Where the trade-desk list went:** `run_classic_console_mode` in `main.rs` + — the reedline REPL that talks to the trade-deal API — seeds it explicitly. + That is the one place it is actually true. +- **Tests:** `tests/completion_schema.rs` (4) loads `data/countries.csv` + through the ordinary loader and asserts suggestions follow from the data; + `tests/datetime_completion.rs` gained the negative cases (a string column + named `tradeDate` must *not* be offered `DateTime(`); 5 unit tests in + `legacy.rs`. +- **Left for T4, already captured:** `cardinality` and `row_count` are + populated and pinned by test — on `countries.csv`, `region` has 5 distinct + values across 250 rows and `name.common` has 250. Nothing reads them yet. +- **Found on the way, not fixed here:** one quoted-empty cell (`""`) in an + otherwise integer column makes the loader store `String("")` rather than + `Null`, which merges the column to `DataType::Mixed`. `independent` in + `countries.csv` is a 0/1 flag that types as string for exactly this reason, + while `unMember` — same shape, no empty cell — types as numeric. That is + upstream type inference and affects more than completion, so it wants its + own number rather than a patch here; `tests/completion_schema.rs` records + the current behaviour so a change is visible. ### T3 — Suggestions are untyped strings -- **Status:** 🔴 OPEN — prerequisite for T4 +- **Status:** 🔴 OPEN — **do this next**; prerequisite for T4 - **Where:** `ParseResult::suggestions: Vec` and every site that builds one - **Observed:** A flat `Vec` cannot express a display label distinct @@ -151,7 +176,7 @@ mechanical but wants doing *before* T4, not as a retrofit. than retrofitting. ### T4 — No value completion for low-cardinality columns -- **Status:** 🔴 OPEN — depends on T2 and T3 +- **Status:** 🔴 OPEN — depends on T3; T2 has landed - **Where:** `detect_cursor_context` in `src/sql/recursive_parser.rs` - **Observed:** `WHERE region = ''` offers nothing. There is `AfterComparisonOp(col, op)` for a cursor *after* an operator, but no context @@ -195,8 +220,8 @@ mechanical but wants doing *before* T4, not as a retrofit. completion that have not been written down. T1 was the first of them to be described precisely enough to fix. - **Action:** as each is hit, give it a T-number rather than working around it. - Worth capturing *before* starting T2, in case any of them changes what belongs - in `ColumnInfo`. + `ColumnInfo` now exists and is cheap to extend, so a new annoyance that wants + another per-column fact is a field addition rather than a redesign. --- @@ -228,3 +253,37 @@ Older, non-living notes that still contain usable thinking: Fold it into T3 rather than doing it twice. - [`NVIM_SMART_COLUMN_COMPLETION.md`](NVIM_SMART_COLUMN_COMPLETION.md) — see T4. - [`DEBUGGING_TUI.md`](DEBUGGING_TUI.md) — F5 debug view. + +### T7 — Residual trade-desk awareness outside the completer +- **Status:** 🔴 OPEN — mostly deletion; do after T3 or whenever +- **Where:** see the survey below +- **Observed:** T2 removed the trade-desk column list from the completer's + *type* decisions, but the TUI still knows what a trade desk is in several + other places. The principle the codebase should hold: **the editor drives + itself entirely from the loaded table's schema and data, and knows nothing + about any particular dataset.** Anything left over is a hack from before + there was a schema to drive from. +- **Survey (2026-09-01), in descending order of how much it matters:** + + | Site | What it does | Disposition | + |---|---|---| + | `src/sql/cursor_aware_parser.rs:77,573` | `get_first_table_name().unwrap_or("trade_deal")` — the default table name when no file is loaded | **Live behaviour.** With an empty schema there is no table; the fallback should be "no columns", not a made-up table name. | + | `src/ui/tui_app.rs:254-256,377-381` | Help panes hardcode `SELECT * FROM trade_deal WHERE counterparty.Contains('Goldman')` etc. | **Live and user-facing** — reachable from `main.rs:1794`. Examples should be generated from the loaded table, or be dataset-neutral. | + | `src/sql/smart_parser.rs` | Five hardcoded `schema.get_columns("trade_deal")` lookups and a `["trade_deal", "instrument"]` table list | **Dead file.** Only reference is `pub mod smart_parser;`. Delete. | + | `src/dynamic_schema.rs` | Its own `TableInfo`, and a `vec!["trade_deal"]` fallback | **Dead file.** Only reference is `pub mod dynamic_schema;`. Also the only caller of `schema_config::load_schema_config()`. Delete. | + | `src/config/schema_config.rs:47` | A default schema whose one table is `trade_deal` | Falls out once `dynamic_schema` goes. | + | `src/config/schema_config.rs:65` | `get_full_trade_deal_columns()` | Keep for now — see below. | + | `src/cli/help.rs:282-285`, `src/main.rs:403-406` | Printed example queries against `trade_deal` | Cosmetic, but same principle. | + +- **The one place it is legitimate:** `run_classic_console_mode` in `main.rs` is + a reedline REPL that talks to a trade-deal API (`api_client.query_trades`), + so *its* schema really is trade_deal — T2 moved the seeding there + deliberately. That is the natural home for + `get_full_trade_deal_columns()`, and it disappears with the classic REPL if + that mode is ever retired. +- **Why it is worth a number rather than a cleanup commit:** two of the five + sites are dead files, and deleting a dead file that mentions `trade_deal` is + easy to mistake for the whole job. The live ones are the two in the table's + first two rows. +- **Not to be confused with T2's leftovers:** the completer's *type* decisions + are already schema-driven. This entry is about the surrounding TUI. diff --git a/src/main.rs b/src/main.rs index f7076d7a..ed5d51ff 100644 --- a/src/main.rs +++ b/src/main.rs @@ -754,8 +754,16 @@ fn run_classic_console_mode() -> io::Result<()> { FileBackedHistory::with_file(50, history_file).expect("Error configuring history"), ); - // Set up SQL completion - let completer = Box::new(SqlCompleter::new()); + // Set up SQL completion. This REPL talks to the trade-deal API + // (`query_trades` below), so its schema really is trade_deal - seeding it + // here, rather than defaulting `Schema::new()` to it, keeps the trade-desk + // column list out of the file-based TUI's completer (T2). + let mut sql_completer = SqlCompleter::new(); + sql_completer.update_schema( + "trade_deal".to_string(), + sql_cli::config::schema_config::get_full_trade_deal_columns(), + ); + let completer = Box::new(sql_completer); let completion_menu = Box::new( ColumnarMenu::default() .with_name("sql_completion") diff --git a/src/sql/cursor_aware_parser.rs b/src/sql/cursor_aware_parser.rs index 831b9352..1e8742bb 100644 --- a/src/sql/cursor_aware_parser.rs +++ b/src/sql/cursor_aware_parser.rs @@ -1,5 +1,5 @@ use crate::data::csv_fixes::quote_if_needed; -use crate::parser::{ParseState, Schema}; +use crate::parser::{ParseState, Schema, TableInfo}; use crate::recursive_parser::{detect_cursor_context, CursorContext, LogicalOp}; use crate::sql::completion_token::{find_completion_token, CompletionToken}; @@ -51,6 +51,13 @@ impl CursorAwareParser { self.schema.set_single_table(&table_name, columns); } + /// Replace the schema with a fully-typed snapshot of the loaded table. + /// Preferred over [`Self::update_single_table`], which can only say + /// "string" about every column. + pub fn update_single_table_info(&mut self, table: TableInfo) { + self.schema.set_single_table_info(table); + } + #[must_use] pub fn get_table_columns(&self, table_name: &str) -> Vec { self.schema.get_columns(table_name) @@ -220,6 +227,7 @@ impl CursorAwareParser { // For numbers, no specific suggestions vec![] } + "boolean" => vec!["true".to_string(), "false".to_string()], _ => vec![], }; (suggestions, format!("AfterComparison({col_name} {op})")) @@ -709,147 +717,18 @@ impl CursorAwareParser { selected_columns } - fn detect_method_call_context( - &self, - query_before_cursor: &str, - _cursor_pos: usize, - ) -> Option<(String, String)> { - // Look for pattern: "propertyName." at the end of the query before cursor - // This handles cases like "WHERE platformOrderId." or "SELECT COUNT(*) WHERE ticker." - // But NOT cases like "WHERE prop.Contains('x') AND " where we've moved past the method call - - // Find the last dot before cursor - if let Some(dot_pos) = query_before_cursor.rfind('.') { - // Check if cursor is close to the dot - if there's too much text after the dot, - // we're probably not in method call context anymore - let text_after_dot = &query_before_cursor[dot_pos + 1..]; - - // If there's significant text after the dot that looks like a completed method call, - // we're probably not in method call context - if text_after_dot.contains(')') - && (text_after_dot.contains(" AND ") - || text_after_dot.contains(" OR ") - || text_after_dot.trim().ends_with(" AND") - || text_after_dot.trim().ends_with(" OR")) - { - return None; // We've completed the method call and moved on - } - - // Extract the word immediately before the dot - let before_dot = &query_before_cursor[..dot_pos]; - - // Find the start of the property name (going backwards from dot) - let mut property_start = dot_pos; - let chars: Vec = before_dot.chars().collect(); - - while property_start > 0 { - let char_pos = property_start - 1; - if char_pos < chars.len() { - let ch = chars[char_pos]; - if ch.is_alphanumeric() || ch == '_' { - property_start -= 1; - } else { - break; - } - } else { - break; - } - } - - if property_start < dot_pos { - let property_name = before_dot[property_start..].trim().to_string(); - - // Check if this property exists in our schema and get its type - if let Some(property_type) = self.get_property_type(&property_name) { - return Some((property_name, property_type)); - } - } - } - - None - } - + /// The completion category of a column, from the loaded schema. + /// + /// Before T2 this was a hardcoded list of trade-desk column names with + /// `else => "string"`, so on any other dataset every column was a string: + /// numeric columns were offered `Contains('')` and date columns never got + /// `DateTime(`. `None` now means genuinely unknown - no file loaded yet, + /// or text that is not a column at all - and callers still fall back to + /// string methods there, which is the safe default for an unknown name. fn get_property_type(&self, property_name: &str) -> Option { - // Get property type from schema - for now, we'll use a simple mapping - // In a more sophisticated implementation, this would query the actual schema - - let property_lower = property_name.to_lowercase(); - - // String properties (most common for Dynamic LINQ operations) - let string_properties = [ - "platformorderid", - "dealid", - "externalorderid", - "parentorderid", - "instrumentid", - "instrumentname", - "instrumenttype", - "isin", - "cusip", - "ticker", - "exchange", - "counterparty", - "counterpartyid", - "counterpartytype", - "counterpartycountry", - "trader", - "portfolio", - "strategy", - "desk", - "status", - "confirmationstatus", - "settlementstatus", - "allocationstatus", - "currency", - "side", - "producttype", - "venue", - "clearinghouse", - "prime", - "comments", - "book", - "source", - "sourcesystem", - ]; - - // Numeric properties - let numeric_properties = [ - "price", - "quantity", - "notional", - "commission", - "accrual", - "netamount", - "accruedinterest", - "grossamount", - "settlementamount", - "fees", - "tax", - ]; - - // DateTime properties - let datetime_properties = [ - "tradedate", - "settlementdate", - "createddate", - "modifieddate", - "valuedate", - "maturitydate", - "confirmationdate", - "executiondate", - "lastmodifieddate", - ]; - - if string_properties.contains(&property_lower.as_str()) { - Some("string".to_string()) - } else if numeric_properties.contains(&property_lower.as_str()) { - Some("numeric".to_string()) - } else if datetime_properties.contains(&property_lower.as_str()) { - Some("datetime".to_string()) - } else { - // Default to string for unknown properties - Some("string".to_string()) - } + self.schema + .find_column(property_name) + .map(|column| column.data_type.as_str().to_string()) } /// Find a safe UTF-8 character boundary at or before the given position @@ -990,8 +869,33 @@ impl CursorAwareParser { mod tests { use super::*; + use crate::parser::ColumnInfo; + use crate::parser::ColumnType; + + /// The trade_deal schema these tests were written against. It used to be + /// the *default* schema, which is exactly what T2 removed - a parser with + /// no file loaded now knows no columns, so the fixture has to say so. fn create_test_parser() -> CursorAwareParser { - CursorAwareParser::new() + let mut parser = CursorAwareParser::new(); + parser.update_single_table_info(TableInfo::new( + "trade_deal", + crate::config::schema_config::get_full_trade_deal_columns() + .into_iter() + .map(|name| { + let column_type = match name.to_lowercase().as_str() { + "price" | "quantity" | "notional" | "commission" | "netamount" => { + ColumnType::Numeric + } + "tradedate" | "settlementdate" | "createddate" | "confirmationdate" => { + ColumnType::DateTime + } + _ => ColumnType::String, + }; + ColumnInfo::new(name).with_type(column_type) + }) + .collect(), + )); + parser } #[test] diff --git a/src/sql/hybrid_parser.rs b/src/sql/hybrid_parser.rs index 5c150b70..f7fa6c6a 100644 --- a/src/sql/hybrid_parser.rs +++ b/src/sql/hybrid_parser.rs @@ -1,4 +1,5 @@ use crate::cursor_aware_parser::CursorAwareParser; +use crate::parser::TableInfo; use crate::recursive_parser::{detect_cursor_context, tokenize_query, CursorContext, LogicalOp}; #[derive(Clone)] @@ -45,6 +46,11 @@ impl HybridParser { self.parser.update_single_table(table_name, columns); } + /// Replace the schema with a fully-typed snapshot of the loaded table. + pub fn update_single_table_info(&mut self, table: TableInfo) { + self.parser.update_single_table_info(table); + } + #[must_use] pub fn get_table_columns(&self, table_name: &str) -> Vec { self.parser.get_table_columns(table_name) diff --git a/src/sql/parser/legacy.rs b/src/sql/parser/legacy.rs index 6d0fa9e4..25799bc1 100644 --- a/src/sql/parser/legacy.rs +++ b/src/sql/parser/legacy.rs @@ -3,7 +3,7 @@ //! These types were previously defined in parser.rs and are needed //! by various parts of the codebase. -use crate::config::schema_config; +use crate::data::datatable::{DataColumn, DataType}; #[derive(Debug, Clone, PartialEq)] pub enum SqlToken { @@ -115,7 +115,103 @@ impl SqlParser { } } -#[derive(Debug, Clone)] +/// The type categories completion actually branches on. +/// +/// Deliberately coarser than [`DataType`]: the editor only decides which +/// methods and which literal shapes to offer, and `Integer` vs `Float` makes +/// no difference to either. Keeping it separate is also what keeps the schema +/// a *snapshot* rather than a view onto the loaded table (see [`Schema`]). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ColumnType { + String, + Numeric, + DateTime, + Boolean, +} + +impl ColumnType { + /// The wire name used by the completion paths that still branch on strings + /// (`get_string_method_suggestions`, the `AfterComparisonOp` arm). + #[must_use] + pub fn as_str(self) -> &'static str { + match self { + ColumnType::String => "string", + ColumnType::Numeric => "numeric", + ColumnType::DateTime => "datetime", + ColumnType::Boolean => "boolean", + } + } +} + +impl From<&DataType> for ColumnType { + fn from(data_type: &DataType) -> Self { + match data_type { + DataType::Integer | DataType::Float => ColumnType::Numeric, + DataType::DateTime => ColumnType::DateTime, + DataType::Boolean => ColumnType::Boolean, + // `Null` (a column empty in every row) and `Mixed` both behave + // like text as far as completion is concerned. + DataType::String | DataType::Null | DataType::Mixed => ColumnType::String, + } + } +} + +/// What the completer knows about one column. +/// +/// A bounded snapshot taken at load time, never a live handle to the +/// `DataTable`. Completion staying a pure function of +/// `(query, cursor, schema)` is what makes its tests cheap to write. +#[derive(Debug, Clone, PartialEq)] +pub struct ColumnInfo { + pub name: String, + pub data_type: ColumnType, + /// Distinct non-null values seen at load time, when the loader counted + /// them. The numerator of a low-cardinality gate; [`TableInfo::row_count`] + /// is the denominator. + pub cardinality: Option, + pub nullable: bool, +} + +impl ColumnInfo { + /// A name-only column. For the callers that genuinely have nothing else - + /// the reedline completer, tests - which therefore keep the string-typed + /// behaviour the whole completer used to have. + pub fn new(name: impl Into) -> Self { + Self { + name: name.into(), + data_type: ColumnType::String, + cardinality: None, + nullable: true, + } + } + + #[must_use] + pub fn with_type(mut self, data_type: ColumnType) -> Self { + self.data_type = data_type; + self + } + + #[must_use] + pub fn with_cardinality(mut self, cardinality: usize) -> Self { + self.cardinality = Some(cardinality); + self + } + + /// Take the snapshot. `DataTable::infer_column_types()` already populates + /// every field this reads, on every load path, so this is the whole of the + /// data-side wiring. + #[must_use] + pub fn from_data_column(column: &DataColumn) -> Self { + Self { + name: column.name.clone(), + data_type: ColumnType::from(&column.data_type), + cardinality: column.unique_values, + nullable: column.nullable, + } + } +} + +#[derive(Debug, Clone, Default)] pub struct Schema { tables: Vec, } @@ -123,69 +219,106 @@ pub struct Schema { #[derive(Debug, Clone)] pub struct TableInfo { pub name: String, - pub columns: Vec, + pub columns: Vec, + /// Rows the column snapshot was taken over, when known. The denominator + /// for a cardinality *ratio*: an absolute count alone cannot tell + /// "5 regions across 250 rows" from "5 rows, every value distinct". + pub row_count: Option, } -impl Default for Schema { - fn default() -> Self { - Self::new() +impl TableInfo { + pub fn new(name: impl Into, columns: Vec) -> Self { + Self { + name: name.into(), + columns, + row_count: None, + } + } + + /// Build a table from column names alone - every column types as string, + /// which is what the completer assumed unconditionally before T2. + pub fn from_names(name: impl Into, columns: Vec) -> Self { + Self::new(name, columns.into_iter().map(ColumnInfo::new).collect()) + } + + #[must_use] + pub fn with_row_count(mut self, row_count: usize) -> Self { + self.row_count = Some(row_count); + self + } + + pub fn column_names(&self) -> Vec { + self.columns.iter().map(|c| c.name.clone()).collect() + } + + pub fn find_column(&self, column_name: &str) -> Option<&ColumnInfo> { + self.columns + .iter() + .find(|c| c.name.eq_ignore_ascii_case(column_name)) } } impl Schema { + /// An empty schema. There is deliberately no built-in table: a completer + /// that suggests trade-desk columns before a file has loaded is wrong on + /// every dataset but one. #[must_use] pub fn new() -> Self { - // Use the complete column list from schema_config - let trade_deal_columns = schema_config::get_full_trade_deal_columns(); - - Self { - tables: vec![ - TableInfo { - name: "trade_deal".to_string(), - columns: trade_deal_columns, - }, - TableInfo { - name: "test".to_string(), - columns: vec![ - "id".to_string(), - "name".to_string(), - "value".to_string(), - "timestamp".to_string(), - ], - }, - ], - } + Self { tables: Vec::new() } } pub fn get_table_names(&self) -> Vec { self.tables.iter().map(|t| t.name.clone()).collect() } - pub fn get_columns_for_table(&self, table_name: &str) -> Vec { + pub fn get_table(&self, table_name: &str) -> Option<&TableInfo> { self.tables .iter() .find(|t| t.name.eq_ignore_ascii_case(table_name)) - .map(|t| t.columns.clone()) + } + + pub fn get_columns_for_table(&self, table_name: &str) -> Vec { + self.get_table(table_name) + .map(TableInfo::column_names) .unwrap_or_default() } + /// The typed columns of one table; empty if it is not loaded. + #[must_use] + pub fn get_column_infos(&self, table_name: &str) -> &[ColumnInfo] { + self.get_table(table_name) + .map_or(&[][..], |t| t.columns.as_slice()) + } + + /// Look a column up by name across every table. + /// + /// Completion contexts such as `price.` carry a bare column name with + /// no table qualifier, so there is nothing to scope the lookup by. In the + /// single-table case the TUI actually runs in, this is exact. + pub fn find_column(&self, column_name: &str) -> Option<&ColumnInfo> { + self.tables.iter().find_map(|t| t.find_column(column_name)) + } + pub fn get_all_columns(&self) -> Vec { - let mut all_columns = Vec::new(); - for table in &self.tables { - all_columns.extend(table.columns.clone()); - } + let mut all_columns: Vec = self + .tables + .iter() + .flat_map(|t| t.columns.iter().map(|c| c.name.clone())) + .collect(); all_columns.sort(); all_columns.dedup(); all_columns } + /// Replace the schema with a single fully-typed table. + pub fn set_single_table_info(&mut self, table: TableInfo) { + self.tables.clear(); + self.tables.push(table); + } + // Legacy compatibility methods pub fn set_single_table(&mut self, table_name: &str, columns: Vec) { - self.tables.clear(); - self.tables.push(TableInfo { - name: table_name.to_string(), - columns, - }); + self.set_single_table_info(TableInfo::from_names(table_name, columns)); } pub fn get_columns(&self, table_name: &str) -> Vec { @@ -196,15 +329,75 @@ impl Schema { self.tables.first().map(|t| t.name.clone()) } + pub fn add_table_info(&mut self, table: TableInfo) { + self.tables.retain(|t| t.name != table.name); + self.tables.push(table); + } + pub fn add_table(&mut self, name: String, columns: Vec) { - // Remove table if it already exists - self.tables.retain(|t| t.name != name); - self.tables.push(TableInfo { name, columns }); + self.add_table_info(TableInfo::from_names(name, columns)); } pub fn has_table(&self, table_name: &str) -> bool { - self.tables - .iter() - .any(|t| t.name.eq_ignore_ascii_case(table_name)) + self.get_table(table_name).is_some() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn new_schema_is_empty() { + // Before T2 this returned the trade_deal schema, so the completer + // suggested `platformOrderId` on a freshly opened CSV. + let schema = Schema::new(); + assert!(schema.get_table_names().is_empty()); + assert!(schema.get_all_columns().is_empty()); + } + + #[test] + fn column_lookup_is_case_insensitive() { + let mut schema = Schema::new(); + schema.set_single_table_info(TableInfo::new("countries", vec![ColumnInfo::new("region")])); + + assert!(schema.find_column("REGION").is_some()); + assert!(schema.find_column("Region").is_some()); + assert!(schema.find_column("regions").is_none()); + } + + #[test] + fn data_types_collapse_to_completion_categories() { + assert_eq!(ColumnType::from(&DataType::Integer), ColumnType::Numeric); + assert_eq!(ColumnType::from(&DataType::Float), ColumnType::Numeric); + assert_eq!(ColumnType::from(&DataType::DateTime), ColumnType::DateTime); + assert_eq!(ColumnType::from(&DataType::Boolean), ColumnType::Boolean); + assert_eq!(ColumnType::from(&DataType::Mixed), ColumnType::String); + assert_eq!(ColumnType::from(&DataType::Null), ColumnType::String); + } + + #[test] + fn snapshot_carries_type_cardinality_and_nullability() { + let mut column = DataColumn::new("region").with_type(DataType::String); + column.unique_values = Some(5); + column.nullable = false; + + let info = ColumnInfo::from_data_column(&column); + assert_eq!(info.name, "region"); + assert_eq!(info.data_type, ColumnType::String); + assert_eq!(info.cardinality, Some(5)); + assert!(!info.nullable); + } + + #[test] + fn name_only_tables_still_work() { + let mut schema = Schema::new(); + schema.set_single_table("t", vec!["a".to_string(), "b".to_string()]); + + assert_eq!(schema.get_columns("t"), vec!["a", "b"]); + assert_eq!( + schema.find_column("a").map(|c| c.data_type), + Some(ColumnType::String) + ); } } diff --git a/src/sql/parser/mod.rs b/src/sql/parser/mod.rs index ac9f9b09..49d4c2e7 100644 --- a/src/sql/parser/mod.rs +++ b/src/sql/parser/mod.rs @@ -23,7 +23,9 @@ pub use ast::{ pub use lexer::{Lexer, LexerMode, Token}; // Re-export legacy types for backward compatibility -pub use legacy::{ParseContext, ParseState, Schema, SqlParser, SqlToken, TableInfo}; +pub use legacy::{ + ColumnInfo, ColumnType, ParseContext, ParseState, Schema, SqlParser, SqlToken, TableInfo, +}; // Test modules #[cfg(test)] diff --git a/src/sql/recursive_parser.rs b/src/sql/recursive_parser.rs index cf556b72..cc58c305 100644 --- a/src/sql/recursive_parser.rs +++ b/src/sql/recursive_parser.rs @@ -8,7 +8,9 @@ pub use super::parser::ast::{ SortDirection, SqlExpression, TableFunction, TableSource, WebCTESpec, WhenBranch, WhereClause, WindowFrame, WindowSpec, CTE, }; -pub use super::parser::legacy::{ParseContext, ParseState, Schema, SqlParser, SqlToken, TableInfo}; +pub use super::parser::legacy::{ + ColumnInfo, ColumnType, ParseContext, ParseState, Schema, SqlParser, SqlToken, TableInfo, +}; pub use super::parser::lexer::{Lexer, LexerMode, Token}; pub use super::parser::ParserConfig; diff --git a/src/ui/state/state_coordinator.rs b/src/ui/state/state_coordinator.rs index 487ccaae..7ef5a87c 100644 --- a/src/ui/state/state_coordinator.rs +++ b/src/ui/state/state_coordinator.rs @@ -7,6 +7,7 @@ use crate::buffer::{AppMode, Buffer, BufferAPI, BufferManager}; use crate::config::config::Config; use crate::data::data_view::DataView; use crate::sql::hybrid_parser::HybridParser; +use crate::sql::parser::{ColumnInfo, TableInfo}; use crate::ui::viewport_manager::ViewportManager; use crate::widgets::search_modes_widget::SearchMode; @@ -28,6 +29,30 @@ pub struct StateCoordinator { pub hybrid_parser: HybridParser, } +/// Snapshot a loaded table's columns for the completer. +/// +/// The parser gets a bounded copy - names, inferred types, distinct counts - +/// and never a handle to the `DataView`. Keeping completion a pure function of +/// `(query, cursor, schema)` is what makes its tests cheap to write, and the +/// snapshot is what T2 needed: `infer_column_types()` has already computed all +/// of this on every load path, and this wiring used to throw it away and pass +/// `Vec`. +/// +/// Columns come from the *source* table rather than the view, so hiding a +/// column in the TUI does not make it uncompletable. +fn schema_snapshot(table_name: &str, dataview: &DataView) -> TableInfo { + let source = dataview.source(); + TableInfo::new( + table_name, + source + .columns + .iter() + .map(ColumnInfo::from_data_column) + .collect(), + ) + .with_row_count(source.row_count()) +} + impl StateCoordinator { // ========== STATIC METHODS FOR DELEGATION ========== // These methods work with references and can be called without owning the components @@ -61,14 +86,14 @@ impl StateCoordinator { pub fn update_parser_with_refs(state_container: &AppStateContainer, parser: &mut HybridParser) { if let Some(dataview) = state_container.get_buffer_dataview() { let table_name = dataview.source().name.clone(); - let columns = dataview.source().column_names(); + let table = schema_snapshot(&table_name, &dataview); debug!( - "StateCoordinator: Updating parser with {} columns for table '{}'", - columns.len(), + "StateCoordinator: Updating parser with {} typed columns for table '{}'", + table.columns.len(), table_name ); - parser.update_single_table(table_name, columns); + parser.update_single_table_info(table); } } @@ -145,14 +170,14 @@ impl StateCoordinator { // Update parser schema from DataView if let Some(dataview) = self.state_container.get_buffer_dataview() { let table_name = dataview.source().name.clone(); - let columns = dataview.source().column_names(); + let table = schema_snapshot(&table_name, &dataview); debug!( - "StateCoordinator: Updating parser with {} columns for table '{}'", - columns.len(), + "StateCoordinator: Updating parser with {} typed columns for table '{}'", + table.columns.len(), table_name ); - self.hybrid_parser.update_single_table(table_name, columns); + self.hybrid_parser.update_single_table_info(table); } } @@ -631,8 +656,7 @@ impl StateCoordinator { .current() .and_then(|b| b.get_dataview()) { - let columns = dataview.column_names(); - parser.update_single_table(table_name.to_string(), columns); + parser.update_single_table_info(schema_snapshot(table_name, dataview)); // Set status message let display_msg = if raw_table_name == table_name { diff --git a/tests/completion_schema.rs b/tests/completion_schema.rs new file mode 100644 index 00000000..cb34a3af --- /dev/null +++ b/tests/completion_schema.rs @@ -0,0 +1,134 @@ +//! The completer's schema, taken from real data (T2). +//! +//! Before T2 the parser was handed `Vec` and decided every column's +//! type from a hardcoded list of trade-desk names. These tests load +//! `data/countries.csv` through the ordinary loader, take the same snapshot +//! the TUI takes, and check that the suggestions follow from the *data*. +//! +//! Nothing here needs a terminal: the parser is a pure function of +//! `(query, cursor, schema)`, which is exactly the property the snapshot +//! boundary exists to preserve. + +use sql_cli::data::datatable::DataTable; +use sql_cli::data::datatable_loaders::load_csv_to_datatable; +use sql_cli::sql::cursor_aware_parser::CursorAwareParser; +use sql_cli::sql::parser::{ColumnInfo, ColumnType, TableInfo}; + +fn countries() -> DataTable { + load_csv_to_datatable("data/countries.csv", "countries").expect("load data/countries.csv") +} + +/// The same snapshot `StateCoordinator::schema_snapshot` takes. +fn snapshot(table: &DataTable) -> TableInfo { + TableInfo::new( + table.name.clone(), + table + .columns + .iter() + .map(ColumnInfo::from_data_column) + .collect(), + ) + .with_row_count(table.row_count()) +} + +fn parser_for(table: &DataTable) -> CursorAwareParser { + let mut parser = CursorAwareParser::new(); + parser.update_single_table_info(snapshot(table)); + parser +} + +#[test] +fn snapshot_types_columns_from_the_loaded_data() { + let table = countries(); + let info = snapshot(&table); + + let column_type = |name: &str| { + info.find_column(name) + .unwrap_or_else(|| panic!("no column {name} in countries.csv")) + .data_type + }; + + // Numbers and text, neither of which appears on any hardcoded list of + // trade-desk column names, so before T2 both typed as string. + assert_eq!(column_type("area"), ColumnType::Numeric); + assert_eq!(column_type("region"), ColumnType::String); + assert_eq!(column_type("name.common"), ColumnType::String); + + // `unMember` is a 0/1 flag and types as numeric. `independent` is the + // same shape but has one quoted-empty cell, which the loader stores as + // `String("")` rather than NULL, so the column merges to `Mixed` and the + // snapshot reports string. That is upstream type inference, not the + // completer - recorded here so the difference is visible if it changes. + assert_eq!(column_type("unMember"), ColumnType::Numeric); + assert_eq!(column_type("independent"), ColumnType::String); +} + +#[test] +fn numeric_columns_no_longer_get_offered_string_only_methods() { + let table = countries(); + let parser = parser_for(&table); + + let query = "SELECT * FROM countries WHERE area."; + let result = parser.get_completions(query, query.len()); + + assert!( + result.suggestions.contains(&"ToString()".to_string()), + "area is numeric, expected ToString(): {:?}", + result.suggestions + ); + assert!( + !result.suggestions.contains(&"Trim()".to_string()), + "area is numeric, Trim() is meaningless on it: {:?}", + result.suggestions + ); +} + +#[test] +fn string_columns_keep_their_methods() { + let table = countries(); + let parser = parser_for(&table); + + let query = "SELECT * FROM countries WHERE region."; + let result = parser.get_completions(query, query.len()); + + assert!(result.suggestions.contains(&"Contains('')".to_string())); + assert!(result.suggestions.contains(&"StartsWith('')".to_string())); +} + +/// The snapshot carries what T4's low-cardinality gate will need. It is not +/// used yet - this pins down that the numbers arriving are the real ones, so +/// the gate can be designed against them rather than against a guess. +#[test] +fn snapshot_carries_cardinality_for_the_value_completion_gate() { + let table = countries(); + let info = snapshot(&table); + let rows = info.row_count.expect("row count captured"); + assert!( + rows > 100, + "expected the full country list, got {rows} rows" + ); + + let cardinality = |name: &str| { + info.find_column(name) + .unwrap_or_else(|| panic!("no column {name}")) + .cardinality + .unwrap_or_else(|| panic!("no cardinality for {name}")) + }; + + // The two ends of the gate: `region` is worth offering as values, + // `name.common` is one distinct value per row and never should be. + let region = cardinality("region"); + assert!( + (2..=12).contains(®ion), + "region should be low cardinality, got {region}" + ); + assert!( + cardinality("independent") <= 3, + "independent is a 0/1 flag - a handful of distinct values in the whole file" + ); + assert_eq!( + cardinality("name.common"), + rows, + "every country name is distinct, so the gate must exclude it" + ); +} diff --git a/tests/datetime_completion.rs b/tests/datetime_completion.rs index 61da48c8..d8e3d299 100644 --- a/tests/datetime_completion.rs +++ b/tests/datetime_completion.rs @@ -1,8 +1,21 @@ use sql_cli::sql::cursor_aware_parser::CursorAwareParser; +use sql_cli::sql::parser::{ColumnInfo, ColumnType, TableInfo}; + +/// Datetime completion is now driven by the schema rather than by a hardcoded +/// list of trade-desk column names (T2). These tests therefore have to say +/// what the column *is*; before T2 `createdDate` was datetime because it was +/// spelled that way, and every column on any other dataset was a string. +fn parser_with(columns: Vec) -> CursorAwareParser { + let mut parser = CursorAwareParser::new(); + parser.update_single_table_info(TableInfo::new("trade_deal", columns)); + parser +} #[test] fn test_datetime_completion_after_comparison() { - let parser = CursorAwareParser::new(); + let parser = parser_with(vec![ + ColumnInfo::new("createdDate").with_type(ColumnType::DateTime) + ]); // Test completion after datetime column comparison let result = parser.get_completions("SELECT * FROM trade_deal WHERE createdDate > ", 45); @@ -15,7 +28,9 @@ fn test_datetime_completion_after_comparison() { #[test] fn test_datetime_completion_with_partial() { - let parser = CursorAwareParser::new(); + let parser = parser_with(vec![ + ColumnInfo::new("createdDate").with_type(ColumnType::DateTime) + ]); // Test completion with partial "Date" let result = parser.get_completions("SELECT * FROM trade_deal WHERE createdDate > Date", 49); @@ -27,6 +42,84 @@ fn test_datetime_completion_with_partial() { assert!(result.suggestions.contains(&"DateTime.Now".to_string())); } +/// The other half of T2: a column that merely *looks* like a date is not one. +/// The old name list matched on spelling, so a CSV column called `tradeDate` +/// holding free text was offered `DateTime(`. +#[test] +fn test_datetime_suggestions_follow_the_schema_not_the_name() { + let parser = parser_with(vec![ + ColumnInfo::new("tradeDate").with_type(ColumnType::String) + ]); + + let query = "SELECT * FROM trade_deal WHERE tradeDate > "; + let result = parser.get_completions(query, query.len()); + + assert!(result.context.contains("AfterComparison")); + assert!( + !result.suggestions.contains(&"DateTime(".to_string()), + "a string column must not be offered a DateTime constructor: {:?}", + result.suggestions + ); + assert!(result.suggestions.contains(&"''".to_string())); +} + +/// Numeric columns used to fall through the name list to `string`, so they +/// were offered `Contains('')` ahead of anything numeric. +#[test] +fn test_numeric_column_gets_numeric_methods() { + let parser = parser_with(vec![ + ColumnInfo::new("population").with_type(ColumnType::Numeric) + ]); + + let query = "SELECT * FROM trade_deal WHERE population."; + let result = parser.get_completions(query, query.len()); + + assert!( + result.suggestions.contains(&"ToString()".to_string()), + "numeric columns should offer ToString(): {:?}", + result.suggestions + ); + assert!( + !result.suggestions.contains(&"Trim()".to_string()), + "numeric columns should not offer string-only methods: {:?}", + result.suggestions + ); +} + +/// Boolean columns had no representation at all before T2 - `independent` on +/// `data/countries.csv` is the motivating case. +#[test] +fn test_boolean_column_suggests_literals() { + let parser = parser_with(vec![ + ColumnInfo::new("independent").with_type(ColumnType::Boolean) + ]); + + let query = "SELECT * FROM trade_deal WHERE independent = "; + let result = parser.get_completions(query, query.len()); + + assert!(result.context.contains("AfterComparison")); + assert_eq!( + result.suggestions, + vec!["true".to_string(), "false".to_string()] + ); +} + +/// An unknown name - no file loaded, or text that is not a column - still +/// falls back to string methods rather than offering nothing. +#[test] +fn test_unknown_column_falls_back_to_string_methods() { + let parser = CursorAwareParser::new(); + + let query = "SELECT * FROM whatever WHERE mystery."; + let result = parser.get_completions(query, query.len()); + + assert!( + result.suggestions.contains(&"Contains('')".to_string()), + "unknown columns keep the safe string default: {:?}", + result.suggestions + ); +} + #[test] fn test_datetime_parsing() { use sql_cli::sql::recursive_parser::Parser; diff --git a/tests/main.rs b/tests/main.rs index dfd2f97f..954cfd2a 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -19,6 +19,9 @@ mod datatable_integration_test; #[path = "datetime_completion.rs"] mod datetime_completion; +#[path = "completion_schema.rs"] +mod completion_schema; + #[path = "temp_table_qualified_join_tests.rs"] mod temp_table_qualified_join_tests;