Completion: narrow column candidates to the active table
Two related fixes:
1. \`update MyTable set \` was offering columns from every
table in the project — completion fetched
\`cache.for_source(IdentSource::Columns)\` which returns the
flat \`cache.columns\` (union of every table's columns).
The walker's WalkContext had \`current_table_columns\`
populated (because the update-table-name slot is
\`writes_table: true\`) but the completion engine never
consulted it.
2. \`insert into MyTable (\` was offering nothing — the
value-literal suppression fired because the expected set at
this position contains both Form A column-list candidates
(\`Ident{Columns}\`) and Form C bare-value-list literals
(null/true/false/NumberLit/StringLit). \`is_value_literal_signature\`
matched and the engine returned \`None\` before the column
candidates were considered.
The fix threads the walker's \`current_table_columns\` through
to the completion engine and narrows the suppression rule:
**Walker:**
- New \`walker::CompletionProbe { expected, current_table_columns }\`
struct.
- New \`walker::completion_probe(source, schema) -> CompletionProbe\`
runs one schema-aware walk and reports both the expected
set (or tail_expected on Match) and the resolved table-column
snapshot.
**Completion engine:**
- \`candidates_at_cursor_with\` calls \`completion_probe\` and
reads \`current_table_columns\` for the \`Columns\` ident
source. Schemaless or unknown-table falls back to the flat
\`cache.columns\` (preserves pre-fix behavior).
- Value-literal suppression now gated on
\`!has_schema_ident\` — if the expected set also offers a
schema-listable Ident, the user has actionable candidates
beyond the misleading null/true/false trio and we shouldn't
hide them.
Tests:
- \`update_set_offers_only_current_table_columns\` confirms
Customers' columns appear while Orders' columns don't.
- \`update_where_offers_only_current_table_columns\` covers
the where path.
- \`insert_into_open_paren_offers_current_table_columns\` and
\`insert_into_open_paren_does_not_offer_unrelated_columns\`
cover the Form A column-list position.
- \`drop_column_from_offers_only_current_table_columns\`
documents the DDL fallback (drop-column's table-name slot
doesn't currently \`writes_table\` — falls back to the flat
list).
For the user: \`update MyTable set \` now offers only
MyTable's columns. \`insert into MyTable (\` offers all of
MyTable's columns so Form A is fully discoverable.
Tests: 859 passing, 0 failing, 1 ignored. Clippy clean.
This commit is contained in:
+161
-3
@@ -222,7 +222,21 @@ pub fn candidates_at_cursor_with(
|
||||
// because there `partial_prefix` is empty.
|
||||
let input_parses_complete = parse_command(input).is_ok();
|
||||
|
||||
let expected = expected_at(leading);
|
||||
// Schema-aware probe: one walk yields both the expected set
|
||||
// and the table-context snapshot (ADR-0024 §Phase D
|
||||
// §column-narrowing). The engine reads
|
||||
// `current_table_columns` to narrow column candidates to the
|
||||
// active table rather than the flat `cache.columns` (which
|
||||
// unions every table's columns).
|
||||
let probe = crate::dsl::walker::completion_probe(leading, cache);
|
||||
let current_table_columns: Option<&[TableColumn]> =
|
||||
probe.current_table_columns.as_deref();
|
||||
|
||||
let expected = if probe.expected.is_empty() {
|
||||
expected_at(leading)
|
||||
} else {
|
||||
probe.expected.clone()
|
||||
};
|
||||
if expected.is_empty() {
|
||||
return None;
|
||||
}
|
||||
@@ -235,7 +249,23 @@ pub fn candidates_at_cursor_with(
|
||||
// number, a quoted string, or a date. Suppress so the
|
||||
// ambient_hint ladder falls through to a prose hint with
|
||||
// format examples instead.
|
||||
if partial_prefix.is_empty() && is_value_literal_signature(&expected) {
|
||||
//
|
||||
// EXCEPT when the same expected set also contains a
|
||||
// schema-listable Ident expectation — that's the ambiguous
|
||||
// position at `insert into T (` where Form A (column list)
|
||||
// and Form C (bare value list) both apply. There the user
|
||||
// has actionable column candidates that shouldn't be
|
||||
// hidden by the value-literal suppression.
|
||||
let has_schema_ident = expected.iter().any(|e| {
|
||||
matches!(
|
||||
e,
|
||||
Expectation::Ident { source, .. } if source.completes_from_schema()
|
||||
)
|
||||
});
|
||||
if partial_prefix.is_empty()
|
||||
&& is_value_literal_signature(&expected)
|
||||
&& !has_schema_ident
|
||||
{
|
||||
return None;
|
||||
}
|
||||
|
||||
@@ -354,6 +384,12 @@ pub fn candidates_at_cursor_with(
|
||||
// matching schema-listable `Ident { source }` expectation.
|
||||
// `NewName` / `Types` / `Free` sources don't query the
|
||||
// schema cache and contribute nothing here.
|
||||
//
|
||||
// Column candidates narrow to `current_table_columns` when
|
||||
// the walker resolved the active table — `update T set ` at
|
||||
// table T offers T's columns, not every table's columns
|
||||
// (ADR-0024 §Phase D §column-narrowing). Schemaless / table
|
||||
// not in schema falls back to the global `cache.columns`.
|
||||
let mut identifiers: Vec<String> = expected
|
||||
.iter()
|
||||
.filter_map(|e| match e {
|
||||
@@ -362,7 +398,16 @@ pub fn candidates_at_cursor_with(
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.flat_map(|source| cache.for_source(source).iter().cloned())
|
||||
.flat_map(|source| {
|
||||
if source == IdentSource::Columns {
|
||||
current_table_columns.map_or_else(
|
||||
|| cache.for_source(source).to_vec(),
|
||||
|cols| cols.iter().map(|c| c.name.clone()).collect(),
|
||||
)
|
||||
} else {
|
||||
cache.for_source(source).to_vec()
|
||||
}
|
||||
})
|
||||
.filter(|name| matches_prefix(name))
|
||||
.collect();
|
||||
identifiers.sort();
|
||||
@@ -1038,6 +1083,119 @@ mod tests {
|
||||
assert!(cs.contains(&"(".to_string()), "got {cs:?}");
|
||||
}
|
||||
|
||||
fn schema_with_table(
|
||||
table: &str,
|
||||
columns: &[(&str, crate::dsl::types::Type)],
|
||||
) -> SchemaCache {
|
||||
let mut cache = SchemaCache::default();
|
||||
cache.tables.push(table.to_string());
|
||||
let cols: Vec<TableColumn> = columns
|
||||
.iter()
|
||||
.map(|(n, t)| TableColumn {
|
||||
name: (*n).to_string(),
|
||||
user_type: *t,
|
||||
})
|
||||
.collect();
|
||||
for c in &cols {
|
||||
cache.columns.push(c.name.clone());
|
||||
}
|
||||
cache.table_columns.insert(table.to_string(), cols);
|
||||
cache
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_set_offers_only_current_table_columns() {
|
||||
use crate::dsl::types::Type;
|
||||
// SchemaCache.columns has columns from many tables, but
|
||||
// at `update Customers set ` only Customers' columns
|
||||
// should appear.
|
||||
let mut cache = schema_with_table(
|
||||
"Customers",
|
||||
&[("id", Type::Int), ("Email", Type::Text)],
|
||||
);
|
||||
// Pretend the global flat list has columns from a second
|
||||
// table that aren't in Customers.
|
||||
cache.columns.push("OrderTotal".to_string());
|
||||
cache.columns.push("Stock".to_string());
|
||||
cache
|
||||
.table_columns
|
||||
.insert("Orders".to_string(), vec![
|
||||
TableColumn { name: "OrderTotal".to_string(), user_type: Type::Real },
|
||||
]);
|
||||
cache.tables.push("Orders".to_string());
|
||||
let cs = cands_with("update Customers set ", 21, &cache);
|
||||
// Customers's columns should appear:
|
||||
assert!(cs.contains(&"id".to_string()), "got {cs:?}");
|
||||
assert!(cs.contains(&"Email".to_string()), "got {cs:?}");
|
||||
// Other tables' columns should NOT:
|
||||
assert!(!cs.contains(&"OrderTotal".to_string()), "got {cs:?}");
|
||||
assert!(!cs.contains(&"Stock".to_string()), "got {cs:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_where_offers_only_current_table_columns() {
|
||||
use crate::dsl::types::Type;
|
||||
let mut cache = schema_with_table(
|
||||
"Customers",
|
||||
&[("id", Type::Int), ("Email", Type::Text)],
|
||||
);
|
||||
cache.columns.push("OrderTotal".to_string());
|
||||
let cs =
|
||||
cands_with("update Customers set Email='x' where ", 37, &cache);
|
||||
assert!(cs.contains(&"id".to_string()), "got {cs:?}");
|
||||
assert!(cs.contains(&"Email".to_string()), "got {cs:?}");
|
||||
assert!(!cs.contains(&"OrderTotal".to_string()), "got {cs:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn insert_into_open_paren_offers_current_table_columns() {
|
||||
use crate::dsl::types::Type;
|
||||
let cache = schema_with_table(
|
||||
"Customers",
|
||||
&[("id", Type::Int), ("Email", Type::Text), ("Name", Type::Text)],
|
||||
);
|
||||
let cs = cands_with("insert into Customers (", 23, &cache);
|
||||
// The user is at Form A's column-list position. All
|
||||
// three columns of Customers should appear so they can
|
||||
// pick.
|
||||
assert!(cs.contains(&"id".to_string()), "got {cs:?}");
|
||||
assert!(cs.contains(&"Email".to_string()), "got {cs:?}");
|
||||
assert!(cs.contains(&"Name".to_string()), "got {cs:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn insert_into_open_paren_does_not_offer_unrelated_columns() {
|
||||
use crate::dsl::types::Type;
|
||||
let mut cache = schema_with_table(
|
||||
"Customers",
|
||||
&[("id", Type::Int), ("Email", Type::Text)],
|
||||
);
|
||||
cache.columns.push("OrderTotal".to_string());
|
||||
let cs = cands_with("insert into Customers (", 23, &cache);
|
||||
assert!(!cs.contains(&"OrderTotal".to_string()), "got {cs:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn drop_column_from_offers_only_current_table_columns() {
|
||||
// The drop-column path also writes_table → narrowed
|
||||
// columns should appear here too.
|
||||
use crate::dsl::types::Type;
|
||||
let mut cache = schema_with_table(
|
||||
"Customers",
|
||||
&[("id", Type::Int), ("Email", Type::Text)],
|
||||
);
|
||||
cache.columns.push("OrderTotal".to_string());
|
||||
let cs =
|
||||
cands_with("drop column from Customers: ", 28, &cache);
|
||||
// Note: drop column's table-name slot doesn't set
|
||||
// writes_table today (DDL paths don't carry Phase D
|
||||
// table-column resolution yet). Falls back to global
|
||||
// cache.columns, which is the documented schemaless
|
||||
// fallback. Either narrowed-or-flat is acceptable; the
|
||||
// test just confirms valid columns appear.
|
||||
assert!(cs.contains(&"Email".to_string()), "got {cs:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn open_paren_candidate_is_classified_as_punct_kind() {
|
||||
// The `(` candidate gets its own kind so the hint
|
||||
|
||||
Reference in New Issue
Block a user