Skip to content

Commit 3acadac

Browse files
committed
Did you mean errors
1 parent 53b3c2b commit 3acadac

File tree

4 files changed

+75
-8
lines changed

4 files changed

+75
-8
lines changed

internal-packages/tsql/src/query/printer.test.ts

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2205,6 +2205,22 @@ describe("Unknown column blocking", () => {
22052205
}).toThrow(QueryError);
22062206
});
22072207

2208+
it("should suggest TSQL column name when user types ClickHouse column name", () => {
2209+
// The schema exposes 'created' but the internal ClickHouse column is 'created_at'
2210+
// When user types 'created_at', we should suggest 'created'
2211+
const schema = createSchemaRegistry([runsSchema]);
2212+
const ctx = createPrinterContext({
2213+
organizationId: "org_test",
2214+
projectId: "proj_test",
2215+
environmentId: "env_test",
2216+
schema,
2217+
});
2218+
2219+
expect(() => {
2220+
printQuery("SELECT id, created_at FROM runs", ctx);
2221+
}).toThrow(/Did you mean "created"/);
2222+
});
2223+
22082224
it("should throw error for unknown qualified column (table.column)", () => {
22092225
expect(() => {
22102226
printQuery("SELECT task_runs.unknown_column FROM task_runs");
@@ -2221,9 +2237,7 @@ describe("Unknown column blocking", () => {
22212237

22222238
it("should throw error for unknown column in complex WHERE", () => {
22232239
expect(() => {
2224-
printQuery(
2225-
"SELECT id FROM task_runs WHERE status = 'completed' AND unknown_column > 10"
2226-
);
2240+
printQuery("SELECT id FROM task_runs WHERE status = 'completed' AND unknown_column > 10");
22272241
}).toThrow(QueryError);
22282242
});
22292243
});

internal-packages/tsql/src/query/printer.ts

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2221,6 +2221,12 @@ export class ClickHousePrinter {
22212221
// This is a security issue - block access to unknown columns
22222222
if (this.tableContexts.size > 0) {
22232223
// Only throw if we have tables in context (otherwise might be subquery)
2224+
// Check if the user typed a ClickHouse column name instead of the TSQL name
2225+
const suggestion = this.findTSQLNameForClickHouseName(columnName);
2226+
if (suggestion) {
2227+
throw new QueryError(`Unknown column "${columnName}". Did you mean "${suggestion}"?`);
2228+
}
2229+
22242230
const availableColumns = this.getAvailableColumnNames();
22252231
throw new QueryError(
22262232
`Unknown column "${columnName}". Available columns: ${availableColumns.join(", ")}`
@@ -2242,18 +2248,47 @@ export class ClickHousePrinter {
22422248
return [...new Set(columns)].sort();
22432249
}
22442250

2251+
/**
2252+
* Find the TSQL column name for a given ClickHouse column name.
2253+
* This is used to provide helpful suggestions when users accidentally
2254+
* use internal ClickHouse column names instead of TSQL names.
2255+
*
2256+
* @returns The TSQL column name if found, null otherwise
2257+
*/
2258+
private findTSQLNameForClickHouseName(clickhouseName: string): string | null {
2259+
for (const tableSchema of this.tableContexts.values()) {
2260+
for (const [tsqlName, columnSchema] of Object.entries(tableSchema.columns)) {
2261+
if (columnSchema.clickhouseName === clickhouseName) {
2262+
return tsqlName;
2263+
}
2264+
}
2265+
}
2266+
return null;
2267+
}
2268+
22452269
/**
22462270
* Resolve a column name to its ClickHouse name using the table schema
22472271
*
22482272
* @throws QueryError if the column is not found in the table schema
22492273
*/
2250-
private resolveColumnName(tableSchema: TableSchema, columnName: string, tableAlias?: string): string {
2274+
private resolveColumnName(
2275+
tableSchema: TableSchema,
2276+
columnName: string,
2277+
tableAlias?: string
2278+
): string {
22512279
const columnSchema = tableSchema.columns[columnName];
22522280
if (columnSchema) {
22532281
return columnSchema.clickhouseName || columnSchema.name;
22542282
}
22552283

22562284
// Column not in schema - this is a security issue, block access
2285+
// Check if the user typed a ClickHouse column name instead of the TSQL name
2286+
for (const [tsqlName, colSchema] of Object.entries(tableSchema.columns)) {
2287+
if (colSchema.clickhouseName === columnName) {
2288+
throw new QueryError(`Unknown column "${columnName}". Did you mean "${tsqlName}"?`);
2289+
}
2290+
}
2291+
22572292
const availableColumns = Object.keys(tableSchema.columns).sort().join(", ");
22582293
const tableName = tableAlias || tableSchema.name;
22592294
throw new QueryError(

internal-packages/tsql/src/query/schema.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -615,6 +615,17 @@ describe("Error message sanitization", () => {
615615
expect(sanitized).toBe("SELECT run_id, triggered_at FROM table");
616616
});
617617

618+
it("should NOT replace column names in 'Did you mean' error messages", () => {
619+
// When a user types an internal ClickHouse column name, we show "Did you mean X?"
620+
// The sanitizer should NOT replace the column name in this case, as it would
621+
// turn "Unknown column 'created_at'. Did you mean 'triggered_at'?" into
622+
// "Unknown column 'triggered_at'. Did you mean 'triggered_at'?" which is confusing
623+
const error = 'Unknown column "created_at". Did you mean "triggered_at"?';
624+
const sanitized = sanitizeErrorMessage(error, [runsSchema]);
625+
// Should preserve both the original column name AND the suggestion
626+
expect(sanitized).toBe('Unknown column "created_at". Did you mean "triggered_at"?');
627+
});
628+
618629
it("should prioritize longer matches (table.column before standalone column)", () => {
619630
// This tests that we replace "trigger_dev.task_runs_v2.friendly_id" as a unit,
620631
// not "trigger_dev.task_runs_v2" and then "friendly_id" separately

internal-packages/tsql/src/query/schema.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -857,10 +857,17 @@ export function sanitizeErrorMessage(message: string, schemas: TableSchema[]): s
857857
}
858858

859859
// Step 5: Replace standalone column names (for unqualified references)
860-
// Sort by length descending to replace longer names first
861-
const sortedColumnNames = [...columnNameMap.entries()].sort((a, b) => b[0].length - a[0].length);
862-
for (const [clickhouseName, { tsqlName }] of sortedColumnNames) {
863-
result = replaceAllOccurrences(result, clickhouseName, tsqlName);
860+
// Skip column replacement for "Did you mean" errors - these already have the correct names
861+
// and replacing would turn "Unknown column 'created_at'. Did you mean 'triggered_at'?"
862+
// into "Unknown column 'triggered_at'. Did you mean 'triggered_at'?" which is confusing
863+
if (!result.includes('Did you mean "')) {
864+
// Sort by length descending to replace longer names first
865+
const sortedColumnNames = [...columnNameMap.entries()].sort(
866+
(a, b) => b[0].length - a[0].length
867+
);
868+
for (const [clickhouseName, { tsqlName }] of sortedColumnNames) {
869+
result = replaceAllOccurrences(result, clickhouseName, tsqlName);
870+
}
864871
}
865872

866873
// Step 6: Remove redundant column aliases like "run_id AS run_id"

0 commit comments

Comments
 (0)