From b7928b13228cff9836c8061843c2b15d34f45160 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sun, 30 Aug 2026 18:42:17 +0100 Subject: [PATCH] fix(tui): stop completion mangling column names that need quoting (T1) Completion had two independent backward scanners from the cursor. The parser decided *what* to suggest; extract_partial_word_at_cursor in text_operations.rs independently decided *what span to replace*. They disagreed whenever quotes or dots were involved. On data/countries.csv (name.common, idd.root, translations.ara.official): SELECT name. offered Contains('') instead of the columns SELECT name.com offered nothing at all SELECT "na produced SELECT name.common" (quote eaten) SELECT na produced SELECT "name.commonname.official" So every column that has to be quoted was effectively unreachable by completion, and cycling corrupted the buffer. Introduce src/sql/completion_token.rs: one quote- and dot-aware scanner both halves share, handling an open quote, a cursor just past a closing quote, and a bare identifier where dots are part of the name. Add ParseResult::replace_start so the parser hands the editor the byte span to splice over instead of the editor guessing, threaded through HybridResult and CompletionState so it survives Tab cycling. complete_dotted_column resolves dotted text that prefixes a real column; text matching no column (capital.Con, 1.5, t.name) falls through to the existing method-call handling unchanged. Also replaces ~55 lines of tangled quote-matching in the suggestion filter with a single unquoted comparison, and deletes the now-unused extract_partial_word_at_cursor - leaving a superseded duplicate scanner in place is how this regresses. Adds docs/TUI_FEATURES.md, a T-numbered book of work for the interactive editor alongside the P (parity) and R (refactoring) logs. The TUI has no equivalent of the parity harness, so its defects only surface in use; T2-T6 record what this fix uncovered, including that get_property_type is still a hardcoded list of trade-desk column names. Tests: tests/dotted_column_completion.rs (12) covers both halves - what is suggested and what the buffer ends up containing - plus 9 unit tests for the scanner. Full suite green, no new clippy warnings. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0152wRProARq2WGPss4698iY --- CLAUDE.md | 6 +- docs/TUI_FEATURES.md | 230 ++++++++++++++++++++++++++++++ src/app_state_container.rs | 19 ++- src/sql/completion_token.rs | 228 +++++++++++++++++++++++++++++ src/sql/cursor_aware_parser.rs | 156 ++++++++++++-------- src/sql/hybrid_parser.rs | 4 + src/sql/mod.rs | 1 + src/ui/enhanced_tui.rs | 22 +-- src/ui/utils/mod.rs | 1 - src/ui/utils/text_operations.rs | 94 +++--------- tests/dotted_column_completion.rs | 198 +++++++++++++++++++++++++ tests/main.rs | 3 + 12 files changed, 814 insertions(+), 148 deletions(-) create mode 100644 docs/TUI_FEATURES.md create mode 100644 src/sql/completion_token.rs create mode 100644 tests/dotted_column_completion.rs 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;