diff --git a/CLAUDE.md b/CLAUDE.md index 9c41bd4f..973bab14 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -380,12 +380,16 @@ The script parser is basic - it chunks on `GO` statements, so proper formatting ## 🔗 Quick Links -### Books of Work (read before starting engine changes) +### Books of Work (read before starting engine or TUI changes) - **`docs/SQL_PARITY.md`** — P-numbered log of divergences vs DuckDB (*do we return the right answer?*). Backed by `tests/comparison/` and a CI gate. - **`docs/ENGINE_REFACTORING.md`** — R-numbered log of structural debt (*can we keep changing the engine safely?*). Records why groundwork is sequenced the way it is, and which refactors are deliberately deferred. +- **`docs/TUI_FEATURES.md`** — T-numbered log for the interactive editor + (*is it pleasant to use?*): completion, key handling, ergonomics. The TUI has + no equivalent of the parity harness, so annoyances get logged here rather than + worked around. ### Code - Function Registry: `src/sql/functions/mod.rs` diff --git a/docs/TUI_FEATURES.md b/docs/TUI_FEATURES.md new file mode 100644 index 00000000..1d6dbc13 --- /dev/null +++ b/docs/TUI_FEATURES.md @@ -0,0 +1,230 @@ +# TUI Features — Book of Work + +The durable decision log for the **interactive editor**: completion, key +handling, and the ergonomics of actually driving the thing. The third track +alongside the two engine logs. + +| | Question | Driven by | +|---|---|---| +| [`SQL_PARITY.md`](SQL_PARITY.md) (P-numbers) | *Do we return the right answer?* | Differential testing vs DuckDB | +| [`ENGINE_REFACTORING.md`](ENGINE_REFACTORING.md) (R-numbers) | *Can we keep changing the engine safely?* | Findings from doing the work | +| **This file (T-numbers)** | *Is it pleasant to use?* | Using the TUI on real data | + +## Why this file exists + +Nearly all sustained effort since early 2026 has gone into the engine, because +the parity harness makes engine gaps *visible* — a corpus case flips to DIFFER +and CI complains. The TUI has no equivalent. Its defects surface only when +someone is typing a query, notices something is wrong, and either works around +it or forgets. Several of the entries below had been live for months. + +This log is the substitute for that missing pressure: when something in the +editor is annoying, it gets a T-number rather than a workaround. + +## Scope + +**In:** the query editor and its completion, key handling, navigation +ergonomics, what the status line says. + +**Out:** anything about query *results* being wrong — that is a P-number. The +dividing line is whether a correct engine would still leave the user annoyed. + +## Principles + +1. **The parser owns semantics, the editor owns text.** The editor must never + re-derive what the parser already decided; see T1 for what that cost. +2. **Testable without a terminal.** Every entry here should be verifiable by + calling the parser and the text-splice helper directly, as + `tests/dotted_column_completion.rs` does. Nothing below needs a TUI harness. +3. **Slices ship independently.** Same rule as the R-log: no multi-session + rewrites. +4. **Real data over synthetic.** `data/countries.csv` (76 columns, dotted names + needing quotes, several genuinely low-cardinality) has been more productive + than any hand-built fixture. Prefer it. + +## Status legend + +| Status | Meaning | +|---|---| +| 🔴 OPEN | confirmed weakness, not yet addressed | +| 🟡 IN PROGRESS | mechanism landed, migration outstanding | +| 🟢 DONE | resolved | +| ⚪ ACCEPTED | known, deliberately not changing — rationale recorded | + +## 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. + +--- + +## Open findings + +### T1 — Completion mangles column names that need quoting +- **Status:** 🟢 DONE 2026-08-30 +- **Where:** `src/sql/completion_token.rs` (new), + `src/sql/cursor_aware_parser.rs`, `src/ui/utils/text_operations.rs` +- **Observed:** Completion had **two independent backward scanners** from the + cursor. `detect_cursor_context` / `CursorAwareParser` decided *what* to + suggest; `extract_partial_word_at_cursor` in `text_operations.rs` + independently decided *what span to replace*. They disagreed on quotes and + dots. On `data/countries.csv`: + + | Typed | Was | Now | + |---|---|---| + | `SELECT name.` | `Contains('')`, `StartsWith('')`… | `"name.common"`, `"name.official"` | + | `SELECT name.com` | *nothing at all* | `"name.common"` | + | `SELECT "na` | `SELECT name.common"` — opening quote eaten | `SELECT "name.common"` | + | `SELECT na` | `SELECT "name.commonname.official"` | `SELECT "name.official"` | + +- **Impact:** Any column whose name contains a dot, space or hyphen — i.e. every + column that *has* to be quoted — was effectively unreachable by completion, + and cycling actively corrupted the buffer. +- **Fixed by:** one quote- and dot-aware scanner (`find_completion_token`) that + both halves share, plus `ParseResult::replace_start` — the parser now hands + the editor the byte span to splice over, instead of the editor guessing. Three + token shapes: an open quote (runs from the quote, spaces and dots included), a + cursor just past a closing quote (the whole identifier is the token, so + cycling *replaces*), and a bare identifier where dots are part of the name. + `complete_dotted_column` resolves dotted text that prefixes a real column; + anything that matches no column (`capital.Con`, `1.5`, `t.name`) falls through + to the existing method handling untouched. +- **Why it matters beyond itself:** `replace_start` is the enabling primitive + for T4. A value completion inside `IN ('')` must replace the span + *between the quotes*, which is not an identifier at all — the old scanner + returned `None` there, so cycling would have concatenated values into + `'AmericasAsiaAfrica'`. The same bug in a different hat. +- **Tests:** `tests/dotted_column_completion.rs` (12), covering both halves — + what is suggested *and* what the buffer ends up containing, including the + method-call cases that must not change. `src/sql/completion_token.rs` has 9 + 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 + only. Two consequences: + - `get_property_type()`, which decides string-methods vs `DateTime(`, is 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 + `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. + +### T3 — Suggestions are untyped strings +- **Status:** 🔴 OPEN — prerequisite for T4 +- **Where:** `ParseResult::suggestions: Vec` and every site that builds + one +- **Observed:** A flat `Vec` cannot express a display label distinct + from the inserted text, the kind of thing being suggested (column / function / + keyword / value), or a rank. +- **Impact:** T4 is the first feature that genuinely needs the split — you want + to insert `'Americas'` but *show* `Americas (23 rows)`. Without a `kind`, + values, columns and keywords also cannot be ranked against each other. +- **Shape:** `Suggestion { insert: String, label: String, kind: SuggestionKind, + detail: Option }`. Mechanical but wide; do it **before** T4 rather + than retrofitting. + +### T4 — No value completion for low-cardinality columns +- **Status:** 🔴 OPEN — depends on T2 and T3 +- **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 + for a cursor *inside* a string literal. +- **Why it is worth doing:** on `countries.csv`, `region` has 5 distinct values + and `independent` has 2. Typing those from memory — with exact spelling and + case — is the single most common friction in filtering unfamiliar data. +- **Design:** + - New `CursorContext::InValueLiteral { column, in_list: bool }`. + - `replace_start` = the byte after the opening quote. This is exactly what T1 + made expressible. + - **Cardinality gate:** an absolute cap *and* a ratio, or `name.common` (250 + values, all unique) gets offered and the feature feels broken. Precedent + exists: `advanced_csv_loader` already computes an `is_categorical` flag from + a `cardinality_threshold` config (0.5). + - **Where the values come from — the real decision.** Snapshot distinct values + into the schema at load time for gated columns only, rather than giving the + parser a live `DataView`. `infer_column_types()` already builds the distinct + `HashSet` and throws it away, so capturing it is nearly free; memory is + bounded precisely by the gate; and it preserves the purity property in T2. +- **Prior art in-repo:** the nvim plugin already has a distinct-values / + cardinality feature (`show_distinct_values()`, see + [`NVIM_SMART_COLUMN_COMPLETION.md`](NVIM_SMART_COLUMN_COMPLETION.md) — which + also records that its keybinding got lost). Worth reading before designing the + gate; the two should probably agree on what "low cardinality" means. + +### T5 — `IN (...)` lists do not iterate +- **Status:** 🔴 OPEN — depends on T4 +- **Where:** as T4 +- **Observed:** N/A — this is the feature T4 exists to enable, logged separately + because it is a distinct slice with its own failure mode. +- **Design:** in `WHERE region IN ('Americas', '')`, parse the existing + list and **exclude values already chosen**, then insert `', '` after accepting + so the next Tab continues the list. The dedupe is not optional polish — + without it, cycling re-offers values already in the list and the feature reads + as broken. + +### T6 — Unlogged completion annoyances +- **Status:** 🔴 OPEN — placeholder +- **Observed:** The TUI is used daily and there are known further problems with + 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`. + +--- + +## Notes on the current design + +Things that are true today and worth knowing before touching this area, but that +are not themselves defects: + +- **There is no completion popup.** Tab cycles in place and the status line + reports `Completed: X (2/5 - Tab for next)`. This is a deliberate fit for a + vim-like editor and works well for small suggestion sets. If a picker is ever + wanted, `src/widgets/history_widget.rs` (Ctrl+R) is the precedent. +- **Completion state lives in `AppStateContainer::CompletionState`**, including + `replace_start`, which is held across Tab presses so that cycling replaces the + previous suggestion rather than appending to it. +- **`CompletionManager` (`src/completion_manager.rs`) is not wired to the TUI.** + It is a parallel, simpler implementation reachable from nothing. Either wire + it or delete it — leaving two completion engines is how T1-shaped bugs get + reintroduced. + +## Related documents + +Older, non-living notes that still contain usable thinking: + +- [`feature_request_smart_function_completion.md`](feature_request_smart_function_completion.md) + — parameterless methods complete inconsistently (`.Length` without `()`, + `.ToLower()` with). Its proposed fix — methods carrying their signature rather + than being bare strings — is essentially T3 arriving from the other direction. + 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. diff --git a/src/app_state_container.rs b/src/app_state_container.rs index 4b12ad02..8c7287cd 100644 --- a/src/app_state_container.rs +++ b/src/app_state_container.rs @@ -684,6 +684,10 @@ pub struct CompletionState { pub last_query: String, pub last_cursor_pos: usize, pub is_active: bool, + /// Byte offset in `last_query` where the accepted suggestion is spliced in, + /// as reported by the parser. Held across Tab presses so cycling replaces + /// the previous suggestion instead of appending to it. + pub replace_start: usize, // Statistics pub total_completions: usize, pub last_completion_time: Option, @@ -704,6 +708,7 @@ impl CompletionState { last_query: String::new(), last_cursor_pos: 0, is_active: false, + replace_start: 0, total_completions: 0, last_completion_time: None, } @@ -717,11 +722,12 @@ impl CompletionState { // Keep last_query and last_cursor_pos for context } - /// Set new suggestions - pub fn set_suggestions(&mut self, suggestions: Vec) { + /// Set new suggestions along with the span they replace + pub fn set_suggestions(&mut self, suggestions: Vec, replace_start: usize) { self.is_active = !suggestions.is_empty(); self.suggestions = suggestions; self.current_index = 0; + self.replace_start = replace_start; if self.is_active { self.last_completion_time = Some(std::time::Instant::now()); self.total_completions += 1; @@ -3515,10 +3521,10 @@ impl AppStateContainer { } } - pub fn set_completion_suggestions(&self, suggestions: Vec) { + pub fn set_completion_suggestions(&self, suggestions: Vec, replace_start: usize) { let mut completion = self.completion.borrow_mut(); let count = suggestions.len(); - completion.set_suggestions(suggestions); + completion.set_suggestions(suggestions, replace_start); if count > 0 { if let Some(ref debug_service) = *self.debug_service.borrow() { @@ -3556,6 +3562,11 @@ impl AppStateContainer { self.completion.borrow().is_active } + /// Byte offset the current suggestion replaces from + pub fn completion_replace_start(&self) -> usize { + self.completion.borrow().replace_start + } + pub fn update_completion_context(&self, query: String, cursor_pos: usize) { self.completion .borrow_mut() diff --git a/src/sql/completion_token.rs b/src/sql/completion_token.rs new file mode 100644 index 00000000..9a5a7d64 --- /dev/null +++ b/src/sql/completion_token.rs @@ -0,0 +1,228 @@ +//! Identifier token scanning for tab completion. +//! +//! Completion has two halves that must agree: the parser decides *what* to +//! suggest, and the editor decides *which span of text* the suggestion +//! replaces. Historically each half scanned backwards from the cursor with its +//! own rules, and they disagreed whenever quotes or dots were involved - +//! cycling through `"name.common"` produced `"name.commonname.official"`, and +//! completing `name.com` produced `name."name.common"`. +//! +//! This module is the single scanner both halves use. + +/// The identifier-ish text immediately before the cursor. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct CompletionToken { + /// Byte offset in the query where the token starts. A suggestion replaces + /// `query[start..cursor_pos]`. + pub start: usize, + /// The raw text of the token, quotes included. + pub text: String, + /// True when the token is (or opens) a double-quoted identifier. + pub is_quoted: bool, +} + +impl CompletionToken { + /// The token with any surrounding quotes removed, for matching against + /// schema column names. Doubled `""` inside a quoted identifier collapses + /// back to a single `"`. + #[must_use] + pub fn unquoted(&self) -> String { + if !self.is_quoted { + return self.text.clone(); + } + let inner = self + .text + .strip_prefix('"') + .unwrap_or(&self.text) + .strip_suffix('"') + .unwrap_or_else(|| self.text.strip_prefix('"').unwrap_or(&self.text)); + inner.replace("\"\"", "\"") + } + + /// The segment after the last dot, e.g. `Con` for `name.Con`. Used when the + /// dotted token turns out to be a method call rather than a column name. + #[must_use] + pub fn last_segment(&self) -> Option<(usize, String)> { + let text = &self.text; + text.rfind('.') + .map(|dot| (self.start + dot + 1, text[dot + 1..].to_string())) + } +} + +/// A character that can appear unquoted inside a column reference. +fn is_identifier_char(ch: char) -> bool { + ch.is_alphanumeric() || ch == '_' || ch == '.' +} + +/// Scan backwards from `cursor_pos` for the identifier the user is completing. +/// +/// Handles three shapes: +/// * an open quoted identifier - `SELECT "name.com|` - the token starts at the +/// opening quote and runs to the cursor, spaces and dots included; +/// * a closed quoted identifier the cursor sits just after - `SELECT +/// "name.common"|` - the token is the whole quoted identifier, so a second +/// Tab replaces it rather than appending to it; +/// * a bare identifier - `SELECT name.com|` - dots are part of the token, +/// because `name.common` is one column name, not a method call on `name`. +/// +/// Returns `None` when the cursor is not adjacent to an identifier (e.g. after +/// a space or comma), which means the suggestion is inserted rather than +/// replacing anything. +#[must_use] +pub fn find_completion_token(query: &str, cursor_pos: usize) -> Option { + let cursor_pos = cursor_pos.min(query.len()); + if cursor_pos == 0 || !query.is_char_boundary(cursor_pos) { + return None; + } + let prefix = &query[..cursor_pos]; + + // Walk the prefix tracking quote state so we know whether the cursor is + // inside a quoted identifier, and where the current/last one started. + let mut in_quote = false; + let mut quote_start = 0usize; + let mut last_closed: Option<(usize, usize)> = None; // (start, end_exclusive) + let bytes = prefix.as_bytes(); + let mut i = 0usize; + while i < bytes.len() { + if bytes[i] == b'"' { + if in_quote { + // `""` inside a quoted identifier is an escaped quote. + if i + 1 < bytes.len() && bytes[i + 1] == b'"' { + i += 2; + continue; + } + in_quote = false; + last_closed = Some((quote_start, i + 1)); + } else { + in_quote = true; + quote_start = i; + } + } + i += 1; + } + + if in_quote { + return Some(CompletionToken { + start: quote_start, + text: prefix[quote_start..].to_string(), + is_quoted: true, + }); + } + + // Cursor immediately after a closing quote: treat the whole quoted + // identifier as the token so cycling replaces it. + if let Some((start, end)) = last_closed { + if end == cursor_pos { + return Some(CompletionToken { + start, + text: prefix[start..].to_string(), + is_quoted: true, + }); + } + } + + // Bare identifier: scan back over identifier chars. + let mut start = cursor_pos; + for (idx, ch) in prefix.char_indices().rev() { + if is_identifier_char(ch) { + start = idx; + } else { + break; + } + } + if start == cursor_pos { + return None; + } + + // A leading run of dots is punctuation, not part of the name. + let text = &prefix[start..]; + let trimmed = text.trim_start_matches('.'); + if trimmed.is_empty() { + return None; + } + let start = start + (text.len() - trimmed.len()); + + Some(CompletionToken { + start, + text: prefix[start..].to_string(), + is_quoted: false, + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn tok(query: &str) -> Option { + find_completion_token(query, query.len()) + } + + #[test] + fn bare_identifier() { + let t = tok("SELECT na").unwrap(); + assert_eq!(t.start, 7); + assert_eq!(t.text, "na"); + assert!(!t.is_quoted); + } + + #[test] + fn dotted_identifier_is_one_token() { + let t = tok("SELECT name.com").unwrap(); + assert_eq!(t.start, 7); + assert_eq!(t.text, "name.com"); + assert_eq!(t.last_segment(), Some((12, "com".to_string()))); + } + + #[test] + fn trailing_dot_is_kept() { + let t = tok("SELECT name.").unwrap(); + assert_eq!(t.text, "name."); + assert_eq!(t.last_segment(), Some((12, String::new()))); + } + + #[test] + fn open_quote_starts_the_token() { + let t = tok("SELECT \"name.com").unwrap(); + assert_eq!(t.start, 7); + assert_eq!(t.text, "\"name.com"); + assert!(t.is_quoted); + assert_eq!(t.unquoted(), "name.com"); + } + + #[test] + fn open_quote_spans_spaces() { + let t = tok("SELECT * FROM t WHERE \"Customer Id").unwrap(); + assert_eq!(t.text, "\"Customer Id"); + assert_eq!(t.unquoted(), "Customer Id"); + } + + #[test] + fn closed_quote_before_cursor_is_the_token() { + let t = tok("SELECT \"name.common\"").unwrap(); + assert_eq!(t.start, 7); + assert_eq!(t.text, "\"name.common\""); + assert!(t.is_quoted); + assert_eq!(t.unquoted(), "name.common"); + } + + #[test] + fn closed_quote_further_back_is_not_the_token() { + assert_eq!(tok("SELECT \"name.common\", "), None); + let t = tok("SELECT \"name.common\", ca").unwrap(); + assert_eq!(t.text, "ca"); + assert!(!t.is_quoted); + } + + #[test] + fn no_token_after_whitespace() { + assert_eq!(tok("SELECT "), None); + assert_eq!(tok(""), None); + } + + #[test] + fn escaped_quotes_inside_identifier() { + let t = tok("SELECT \"od\"\"d").unwrap(); + assert!(t.is_quoted); + assert_eq!(t.unquoted(), "od\"d"); + } +} diff --git a/src/sql/cursor_aware_parser.rs b/src/sql/cursor_aware_parser.rs index cca846f1..831b9352 100644 --- a/src/sql/cursor_aware_parser.rs +++ b/src/sql/cursor_aware_parser.rs @@ -1,6 +1,7 @@ use crate::data::csv_fixes::quote_if_needed; use crate::parser::{ParseState, Schema}; use crate::recursive_parser::{detect_cursor_context, CursorContext, LogicalOp}; +use crate::sql::completion_token::{find_completion_token, CompletionToken}; #[derive(Debug, Clone)] pub struct CursorAwareParser { @@ -12,6 +13,20 @@ pub struct ParseResult { pub suggestions: Vec, pub context: String, pub partial_word: Option, + /// Byte offset in the query where an accepted suggestion should be spliced + /// in; it replaces `query[replace_start..cursor_pos]`. The parser owns this + /// because only it knows whether the text before the cursor is a column + /// reference (`name.com` - replace all of it) or a method call on a column + /// (`price.Con` - replace only `Con`). + pub replace_start: usize, +} + +/// Strip the surrounding quotes from a quoted identifier so it can be compared +/// against what the user typed. +fn strip_identifier_quotes(suggestion: &str) -> &str { + suggestion + .strip_prefix('"') + .map_or(suggestion, |rest| rest.strip_suffix('"').unwrap_or(rest)) } impl Default for CursorAwareParser { @@ -54,6 +69,20 @@ impl CursorAwareParser { .get_first_table_name() .unwrap_or("trade_deal".to_string()); + // The identifier the cursor sits in, scanned with quote- and + // dot-awareness. It is the single source of truth for both what we + // filter against and which span an accepted suggestion replaces. + let token = find_completion_token(query, cursor_pos); + + // `name.com` is ambiguous: a column literally called `name.common`, or + // a method call on a column called `name`. Columns win when the text + // actually prefixes one, because completion is the only way to + // discover a dotted column name - methods stay reachable on any column + // whose name really exists. + if let Some(result) = self.complete_dotted_column(token.as_ref(), &default_table) { + return result; + } + let (suggestions, context_str) = match &cursor_context { CursorContext::SelectClause => { // Apply quote_if_needed to column names @@ -267,82 +296,95 @@ impl CursorAwareParser { suggestions, context: format!("{context:?} (partial: {partial_word:?})"), partial_word, + replace_start: token.as_ref().map_or(cursor_pos, |t| t.start), }; } }; - // Filter by partial word if present (but not for method suggestions as they're already filtered) - let mut final_suggestions = suggestions; let is_method_context = matches!( cursor_context, - CursorContext::AfterColumn(_) - | CursorContext::InMethodCall(_, _) - | CursorContext::AfterComparisonOp(_, _) + CursorContext::AfterColumn(_) | CursorContext::InMethodCall(_, _) ); + let is_value_context = matches!(cursor_context, CursorContext::AfterComparisonOp(_, _)); - if let Some(ref partial) = partial_word { - if !is_method_context { - // Only filter non-method suggestions - final_suggestions.retain(|suggestion| { - // Check if we're dealing with a partial quoted identifier - if let Some(partial_without_quote) = partial.strip_prefix('"') { - // User is typing a quoted identifier like "customer - // Remove the opening quote - - // Check if suggestion is a quoted identifier that matches - if suggestion.starts_with('"') - && suggestion.ends_with('"') - && suggestion.len() > 2 - { - // Full quoted identifier like "Customer Id" - let suggestion_without_quotes = &suggestion[1..suggestion.len() - 1]; - suggestion_without_quotes - .to_lowercase() - .starts_with(&partial_without_quote.to_lowercase()) - } else if suggestion.starts_with('"') && suggestion.len() > 1 { - // Partial quoted identifier (shouldn't happen in suggestions but handle it) - let suggestion_without_quote = &suggestion[1..]; - suggestion_without_quote - .to_lowercase() - .starts_with(&partial_without_quote.to_lowercase()) - } else { - // Also check non-quoted suggestions that might need quotes - suggestion - .to_lowercase() - .starts_with(&partial_without_quote.to_lowercase()) - } - } else { - // Normal non-quoted partial (e.g., "customer") - // Handle quoted column names - check if the suggestion starts with a quote - let suggestion_to_check = if suggestion.starts_with('"') - && suggestion.ends_with('"') - && suggestion.len() > 2 - { - // Remove both quotes for comparison (e.g., "Customer Id" -> "Customer Id") - &suggestion[1..suggestion.len() - 1] - } else if suggestion.starts_with('"') && suggestion.len() > 1 { - // Malformed quoted identifier - just strip opening quote - &suggestion[1..] - } else { - suggestion - }; - - // Now compare the cleaned suggestion with the partial - suggestion_to_check - .to_lowercase() - .starts_with(&partial.to_lowercase()) - } + // Method suggestions arrive pre-filtered against the partial method + // name; everything else is filtered here. Suggestions may be quoted + // (`"name.common"`) while the user typed either `na` or `"na`, so both + // sides are compared with quotes stripped. + let mut final_suggestions = suggestions; + if !is_method_context && !is_value_context { + if let Some(needle) = token + .as_ref() + .map(CompletionToken::unquoted) + .filter(|n| !n.is_empty()) + { + let needle = needle.to_lowercase(); + final_suggestions.retain(|s| { + strip_identifier_quotes(s) + .to_lowercase() + .starts_with(&needle) }); } } + // Method names replace only the segment after the dot (`price.Con` -> + // `price.Contains('')`); everything else replaces the whole identifier. + // After a quoted column the dot terminates the token, so the partial + // method is already the whole token (`"name.common".Star` -> `Star`). + let replace_start = if is_method_context { + token.as_ref().map_or(cursor_pos, |t| { + t.last_segment().map_or(t.start, |(start, _)| start) + }) + } else { + token.as_ref().map_or(cursor_pos, |t| t.start) + }; + ParseResult { suggestions: final_suggestions, context: format!("{context_str} (partial: {partial_word:?})"), partial_word, + replace_start, } } + /// Suggest real column names when the text at the cursor prefixes one. + /// + /// Only dotted text reaches here: undotted prefixes are already handled by + /// the clause contexts, whereas a dotted prefix would otherwise be read as + /// a method call and the column would be unreachable by completion. + fn complete_dotted_column( + &self, + token: Option<&CompletionToken>, + table: &str, + ) -> Option { + let token = token?; + let needle = token.unquoted(); + if !needle.contains('.') { + return None; + } + + let needle_lower = needle.to_lowercase(); + let matches: Vec = self + .schema + .get_columns(table) + .into_iter() + .filter(|col| col.to_lowercase().starts_with(&needle_lower)) + .map(|col| quote_if_needed(&col)) + .collect(); + + // No column by that name - leave it to the method-call handling. + if matches.is_empty() { + return None; + } + + Some(ParseResult { + suggestions: matches, + context: format!("DottedColumn (partial: {needle:?})"), + partial_word: Some(token.text.clone()), + replace_start: token.start, + }) + } + fn extract_word_at_cursor(&self, query: &str, cursor_pos: usize) -> Option { if cursor_pos == 0 || cursor_pos > query.len() { return None; diff --git a/src/sql/hybrid_parser.rs b/src/sql/hybrid_parser.rs index 726d458e..5c150b70 100644 --- a/src/sql/hybrid_parser.rs +++ b/src/sql/hybrid_parser.rs @@ -22,6 +22,9 @@ pub struct HybridResult { pub recursive_context: String, pub cursor_position: usize, pub query_complexity: String, + /// Byte offset where an accepted suggestion replaces text; see + /// [`crate::sql::cursor_aware_parser::ParseResult::replace_start`]. + pub replace_start: usize, } impl Default for HybridParser { @@ -75,6 +78,7 @@ impl HybridParser { recursive_context: recursive_context.to_string(), cursor_position: cursor_pos, query_complexity: self.analyze_query_complexity(query), + replace_start: result.replace_start, } } diff --git a/src/sql/mod.rs b/src/sql/mod.rs index 8300c6f5..ebbcadac 100644 --- a/src/sql/mod.rs +++ b/src/sql/mod.rs @@ -6,6 +6,7 @@ pub mod aggregate_functions; pub mod aggregates; pub mod cache; +pub mod completion_token; pub mod cursor_aware_parser; pub mod functions; pub mod generators; diff --git a/src/ui/enhanced_tui.rs b/src/ui/enhanced_tui.rs index b0122204..47cd43e8 100644 --- a/src/ui/enhanced_tui.rs +++ b/src/ui/enhanced_tui.rs @@ -4070,7 +4070,7 @@ impl EnhancedTuiApp { } self.state_container - .set_completion_suggestions(hybrid_result.suggestions); + .set_completion_suggestions(hybrid_result.suggestions, hybrid_result.replace_start); } else if self.state_container.is_completion_active() { // Cycle to next suggestion self.state_container.next_completion(); @@ -4091,28 +4091,34 @@ impl EnhancedTuiApp { } /// Apply a completion suggestion to the input + /// + /// The span to replace comes from the parser rather than being re-derived + /// here: the two used to disagree over quotes and dots, which is how + /// cycling past `"name.common"` produced `"name.commonname.official"`. fn apply_completion_to_input(&mut self, query: &str, cursor_pos: usize, suggestion: &str) { - let partial_word = - crate::ui::utils::text_operations::extract_partial_word_at_cursor(query, cursor_pos); + let replace_start = self.state_container.completion_replace_start(); - if let Some(partial) = partial_word { - self.apply_partial_completion(query, cursor_pos, &partial, suggestion); + if replace_start < cursor_pos && query.is_char_boundary(replace_start) { + self.apply_partial_completion(query, cursor_pos, replace_start, suggestion); } else { self.apply_full_insertion(query, cursor_pos, suggestion); } } - /// Apply completion when we have a partial word to complete + /// Replace `query[replace_start..cursor_pos]` with the suggestion fn apply_partial_completion( &mut self, query: &str, cursor_pos: usize, - partial: &str, + replace_start: usize, suggestion: &str, ) { // Use extracted completion logic let result = crate::ui::utils::text_operations::apply_completion_to_text( - query, cursor_pos, partial, suggestion, + query, + cursor_pos, + replace_start, + suggestion, ); // Use helper to set text and cursor together - this ensures sync diff --git a/src/ui/utils/mod.rs b/src/ui/utils/mod.rs index fea9ef39..191ec019 100644 --- a/src/ui/utils/mod.rs +++ b/src/ui/utils/mod.rs @@ -7,7 +7,6 @@ pub mod text_utils; pub use column_utils::*; pub use enhanced_tui_helpers::*; pub use scroll_utils::*; -// Re-export from text_operations (has extract_partial_word_at_cursor) pub use text_operations::*; // Re-export from text_utils except the conflicting function pub use text_utils::{get_cursor_token_position, get_token_at_cursor}; diff --git a/src/ui/utils/text_operations.rs b/src/ui/utils/text_operations.rs index e421451f..cba9810c 100644 --- a/src/ui/utils/text_operations.rs +++ b/src/ui/utils/text_operations.rs @@ -534,56 +534,6 @@ mod tests { // ========== SQL-Specific Text Functions ========== -/// Extract partial word at cursor for SQL completion -/// Handles quoted identifiers and SQL-specific parsing -#[must_use] -pub fn extract_partial_word_at_cursor(query: &str, cursor_pos: usize) -> Option { - if cursor_pos == 0 || cursor_pos > query.len() { - return None; - } - - let chars: Vec = query.chars().collect(); - let mut start = cursor_pos; - let end = cursor_pos; - - // Check if we might be in a quoted identifier - let mut in_quote = false; - - // Find start of word (go backward) - while start > 0 { - let prev_char = chars[start - 1]; - if prev_char == '"' { - // Found a quote, include it and stop - start -= 1; - in_quote = true; - break; - } else if prev_char.is_alphanumeric() || prev_char == '_' || (prev_char == ' ' && in_quote) - { - start -= 1; - } else { - break; - } - } - - // If we found a quote but are in a quoted identifier, - // we need to continue backwards to include the identifier content - if in_quote && start > 0 { - // We've already moved past the quote, now get the content before it - // Actually, we want to include everything from the quote forward - // The logic above is correct - we stop at the quote - } - - // Convert back to byte positions - let start_byte = chars[..start].iter().map(|c| c.len_utf8()).sum(); - let end_byte = chars[..end].iter().map(|c| c.len_utf8()).sum(); - - if start_byte < end_byte { - Some(query[start_byte..end_byte].to_string()) - } else { - None - } -} - /// Result of applying a completion to text #[derive(Debug, Clone)] pub struct CompletionResult { @@ -595,50 +545,40 @@ pub struct CompletionResult { pub description: String, } -/// Apply a completion suggestion to text at cursor position -/// Handles quoted identifiers and smart cursor positioning +/// Splice a completion suggestion over `query[replace_start..cursor_pos]`. +/// +/// `replace_start` comes from the parser (see +/// [`crate::sql::cursor_aware_parser::ParseResult::replace_start`]) because +/// only it knows whether the text before the cursor is a column reference to +/// be replaced wholesale or a method name hanging off one. The suggestion is +/// inserted verbatim - quoting is already decided by the suggestion itself, and +/// the span it replaces includes any opening quote the user typed. #[must_use] pub fn apply_completion_to_text( query: &str, cursor_pos: usize, - partial_word: &str, + replace_start: usize, suggestion: &str, ) -> CompletionResult { - let before_partial = &query[..cursor_pos - partial_word.len()]; + let replace_start = replace_start.min(cursor_pos); + let before = &query[..replace_start]; let after_cursor = &query[cursor_pos..]; - // Handle quoted identifiers - avoid double quotes - let suggestion_to_use = if partial_word.starts_with('"') && suggestion.starts_with('"') { - // The partial already includes the opening quote, so use suggestion without its quote - if suggestion.len() > 1 { - suggestion[1..].to_string() - } else { - suggestion.to_string() - } - } else { - suggestion.to_string() - }; - - let new_query = format!("{before_partial}{suggestion_to_use}{after_cursor}"); + let new_query = format!("{before}{suggestion}{after_cursor}"); // Smart cursor positioning based on function signature - let new_cursor_pos = if suggestion_to_use.ends_with("('')") { + let new_cursor_pos = if suggestion.ends_with("('')") { // Function with parameters - position cursor between the quotes // e.g., Contains('|') where | is cursor - before_partial.len() + suggestion_to_use.len() - 2 - } else if suggestion_to_use.ends_with("()") { - // Parameterless function - position cursor after closing parenthesis - // e.g., Length()| where | is cursor - before_partial.len() + suggestion_to_use.len() + replace_start + suggestion.len() - 2 } else { - // Regular completion (not a function) - position at end - before_partial.len() + suggestion_to_use.len() + replace_start + suggestion.len() }; // Better description based on completion type - let description = if suggestion_to_use.ends_with("('')") { + let description = if suggestion.ends_with("('')") { format!("Completed '{suggestion}' with cursor positioned for parameter input") - } else if suggestion_to_use.ends_with("()") { + } else if suggestion.ends_with("()") { format!("Completed parameterless function '{suggestion}'") } else { format!("Completed '{suggestion}'") diff --git a/tests/dotted_column_completion.rs b/tests/dotted_column_completion.rs new file mode 100644 index 00000000..ffe6acae --- /dev/null +++ b/tests/dotted_column_completion.rs @@ -0,0 +1,198 @@ +//! Tab completion for column names that need quoting. +//! +//! `data/countries.csv` has columns like `name.common` and `idd.root`. Those +//! must be quoted in SQL, and the dot in them collides with method-call syntax +//! (`price.Contains('x')`), so they exercise every corner of the completion +//! path: recognising the column, filtering on a partial, and splicing the +//! suggestion in without mangling the quotes. + +use sql_cli::sql::cursor_aware_parser::CursorAwareParser; +use sql_cli::ui::utils::text_operations::apply_completion_to_text; + +fn parser() -> CursorAwareParser { + let mut parser = CursorAwareParser::new(); + parser.update_single_table( + "countries".to_string(), + vec![ + "name.common".to_string(), + "name.official".to_string(), + "tld".to_string(), + "cca2".to_string(), + "idd.root".to_string(), + "capital".to_string(), + "region".to_string(), + ], + ); + parser +} + +/// Suggestions for the cursor at the end of `query`. +fn suggest(query: &str) -> Vec { + parser().get_completions(query, query.len()).suggestions +} + +/// Type `query`, press Tab, accept `suggestion`; returns the resulting text. +fn complete(query: &str, suggestion: &str) -> String { + let result = parser().get_completions(query, query.len()); + assert!( + result.suggestions.iter().any(|s| s == suggestion), + "{suggestion:?} was not offered for {query:?}; got {:?}", + result.suggestions + ); + apply_completion_to_text(query, query.len(), result.replace_start, suggestion).new_text +} + +// --------------------------------------------------------------------------- +// Recognising dotted column names +// --------------------------------------------------------------------------- + +#[test] +fn bare_prefix_offers_the_quoted_columns() { + assert_eq!( + suggest("SELECT na"), + vec!["\"name.common\"", "\"name.official\""] + ); +} + +#[test] +fn trailing_dot_offers_columns_not_string_methods() { + // `name` is not a column, so `name.` can only be reaching for `name.common` + // or `name.official` - offering Contains()/StartsWith() here is a dead end. + assert_eq!( + suggest("SELECT name."), + vec!["\"name.common\"", "\"name.official\""] + ); +} + +#[test] +fn partial_after_the_dot_narrows_the_columns() { + assert_eq!(suggest("SELECT name.com"), vec!["\"name.common\""]); + assert_eq!(suggest("SELECT name.off"), vec!["\"name.official\""]); +} + +#[test] +fn dotted_columns_are_offered_in_where_and_order_by() { + assert_eq!( + suggest("SELECT * FROM countries WHERE idd."), + vec!["\"idd.root\""] + ); + assert_eq!( + suggest("SELECT * FROM countries ORDER BY name.off"), + vec!["\"name.official\""] + ); +} + +#[test] +fn method_calls_on_real_columns_still_work() { + // `capital` is a real column and nothing is named `capital.*`, so the dot + // means a method call. + let suggestions = suggest("SELECT * FROM countries WHERE capital."); + assert!( + suggestions.contains(&"Contains('')".to_string()), + "expected string methods, got {suggestions:?}" + ); + + let suggestions = suggest("SELECT * FROM countries WHERE \"name.common\".Con"); + assert_eq!(suggestions, vec!["Contains('')"]); +} + +// --------------------------------------------------------------------------- +// Splicing the suggestion in +// --------------------------------------------------------------------------- + +#[test] +fn accepting_a_quoted_column_replaces_the_whole_partial() { + assert_eq!( + complete("SELECT na", "\"name.common\""), + "SELECT \"name.common\"" + ); + // The dot is part of the identifier, not a separator to complete after. + assert_eq!( + complete("SELECT name.com", "\"name.common\""), + "SELECT \"name.common\"" + ); + assert_eq!( + complete("SELECT name.", "\"name.official\""), + "SELECT \"name.official\"" + ); +} + +#[test] +fn a_quote_the_user_typed_is_not_duplicated() { + // Typing the opening quote yourself is the natural way to reach a column + // that needs quoting; it used to produce `SELECT name.common"`. + assert_eq!( + complete("SELECT \"na", "\"name.common\""), + "SELECT \"name.common\"" + ); + assert_eq!( + complete("SELECT \"name.com", "\"name.common\""), + "SELECT \"name.common\"" + ); +} + +#[test] +fn cycling_replaces_the_previous_suggestion() { + // Second Tab: the cursor sits after the closing quote of the suggestion + // just inserted, which must be replaced rather than appended to. + let first = complete("SELECT na", "\"name.common\""); + assert_eq!(first, "SELECT \"name.common\""); + + let result = parser().get_completions(&first, first.len()); + let second = apply_completion_to_text( + &first, + first.len(), + result.replace_start, + "\"name.official\"", + ) + .new_text; + assert_eq!(second, "SELECT \"name.official\""); +} + +#[test] +fn accepting_a_method_keeps_the_column_it_hangs_off() { + assert_eq!( + complete("SELECT * FROM countries WHERE capital.Con", "Contains('')"), + "SELECT * FROM countries WHERE capital.Contains('')" + ); + assert_eq!( + complete( + "SELECT * FROM countries WHERE \"name.common\".", + "Contains('')" + ), + "SELECT * FROM countries WHERE \"name.common\".Contains('')" + ); + // The closing quote ends the identifier, so the partial method after the + // dot is the whole token rather than its last segment. + assert_eq!( + complete( + "SELECT * FROM countries WHERE \"name.common\".Star", + "StartsWith('')" + ), + "SELECT * FROM countries WHERE \"name.common\".StartsWith('')" + ); +} + +#[test] +fn completing_after_a_comma_does_not_disturb_earlier_columns() { + assert_eq!( + complete("SELECT \"name.common\", ca", "capital"), + "SELECT \"name.common\", capital" + ); +} + +#[test] +fn cursor_lands_inside_the_quotes_of_a_method_argument() { + let result = parser().get_completions( + "SELECT * FROM countries WHERE capital.Con", + "SELECT * FROM countries WHERE capital.Con".len(), + ); + let query = "SELECT * FROM countries WHERE capital.Con"; + let applied = + apply_completion_to_text(query, query.len(), result.replace_start, "Contains('')"); + assert_eq!( + &applied.new_text[applied.new_cursor_position..], + "')", + "cursor should sit between the argument quotes" + ); +} diff --git a/tests/main.rs b/tests/main.rs index 0609d904..dfd2f97f 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -219,3 +219,6 @@ mod viewport_manager_test; #[path = "projection_column_width_tests.rs"] mod projection_column_width_tests; + +#[path = "dotted_column_completion.rs"] +mod dotted_column_completion;