Skip to content

Commit 208c6dd

Browse files
committed
Fix for SELECT using JSON paths
1 parent f3cba91 commit 208c6dd

File tree

2 files changed

+28
-13
lines changed

2 files changed

+28
-13
lines changed

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

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -622,19 +622,18 @@ describe("ClickHousePrinter", () => {
622622
expect(sql).not.toContain("error.message.:String");
623623
});
624624

625-
it("should add .:String in SELECT but not in WHERE for same field", () => {
625+
it("should NOT add .:String in SELECT or WHERE when no GROUP BY", () => {
626626
const ctx = createJsonContext();
627627
const { sql } = printQuery(
628628
"SELECT error.data.name FROM runs WHERE error.data.name = 'test'",
629629
ctx
630630
);
631631

632-
// SELECT should have .:String with alias
633-
expect(sql).toContain("error.data.name.:String AS error_data_name");
632+
// SELECT should NOT have .:String (no GROUP BY, so no need for type hint)
633+
expect(sql).toContain("error.data.name AS error_data_name");
634+
expect(sql).not.toContain(".:String");
634635
// WHERE should NOT have .:String
635636
expect(sql).toContain("equals(error.data.name,");
636-
// Make sure WHERE doesn't accidentally get the type hint
637-
expect(sql).not.toMatch(/equals\(error\.data\.name\.:String/);
638637
});
639638

640639
it("should add .:String in GROUP BY but not in WHERE for same query", () => {
@@ -712,13 +711,14 @@ describe("ClickHousePrinter", () => {
712711
expect(sql).toContain("error_text AS error");
713712
});
714713

715-
it("should use JSON column for subfield access", () => {
714+
it("should use JSON column for subfield access without .:String when no GROUP BY", () => {
716715
const ctx = createTextColumnContext();
717716
const { sql } = printQuery("SELECT output.data.name FROM runs", ctx);
718717

719-
// Should use the original JSON column with .:String type hint
720-
expect(sql).toContain("output.data.name.:String");
718+
// Should use the original JSON column without .:String (no GROUP BY)
719+
expect(sql).toContain("output.data.name AS output_data_name");
721720
expect(sql).not.toContain("output_text");
721+
expect(sql).not.toContain(".:String");
722722
});
723723
});
724724

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

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,8 @@ export class ClickHousePrinter {
114114
private outputColumns: OutputColumnMetadata[] = [];
115115
/** Whether we're currently processing GROUP BY expressions */
116116
private inGroupByContext = false;
117+
/** Whether the current query has a GROUP BY clause (used for JSON subfield type hints) */
118+
private queryHasGroupBy = false;
117119
/** Columns hidden when SELECT * is expanded to core columns only */
118120
private hiddenColumns: string[] = [];
119121
/**
@@ -392,6 +394,11 @@ export class ClickHousePrinter {
392394
}
393395
}
394396

397+
// Track if query has GROUP BY for JSON subfield type hint decisions
398+
// (ClickHouse requires .:String for Dynamic types in GROUP BY, and SELECT must match)
399+
const savedQueryHasGroupBy = this.queryHasGroupBy;
400+
this.queryHasGroupBy = !!node.group_by;
401+
395402
// Process SELECT columns and collect metadata
396403
// Using flatMap because asterisk expansion can return multiple columns
397404
// Set inProjectionContext to block internal-only columns in user projections
@@ -543,6 +550,7 @@ export class ClickHousePrinter {
543550

544551
// Restore saved contexts (for nested queries)
545552
this.selectAliases = savedAliases;
553+
this.queryHasGroupBy = savedQueryHasGroupBy;
546554
this.tableContexts = savedTableContexts;
547555
this.allowedInternalColumns = savedInternalColumns;
548556
this.internalOnlyColumns = savedInternalOnlyColumns;
@@ -2124,14 +2132,21 @@ export class ClickHousePrinter {
21242132
let result = resolvedChain.map((part) => this.printIdentifierOrIndex(part)).join(".");
21252133

21262134
// For JSON column subfield access (e.g., error.data.name), add .:String type hint
2127-
// This is required because ClickHouse's Dynamic/Variant types are not allowed in
2128-
// GROUP BY without type casting, and SELECT/GROUP BY expressions must match
2129-
// However, we skip this in WHERE comparisons where it breaks the query
2135+
// This is ONLY required when the query has GROUP BY, because:
2136+
// 1. ClickHouse's Dynamic/Variant types are not allowed in GROUP BY without type casting
2137+
// 2. SELECT/GROUP BY expressions must match
2138+
// For queries without GROUP BY, the .:String type hint actually breaks the query
2139+
// (returns NULL instead of the actual value)
2140+
// We also skip this in WHERE comparisons where it breaks the query
21302141
if (resolvedChain.length > 1) {
21312142
// Check if the root column (first part) is a JSON column
21322143
const rootColumnSchema = this.resolveFieldToColumnSchema([node.chain[0]]);
2133-
// Add .:String for SELECT/GROUP BY, but NOT in WHERE comparisons
2134-
if (rootColumnSchema?.type === "JSON" && !this.isInWhereComparisonContext()) {
2144+
// Add .:String ONLY for GROUP BY queries, and NOT in WHERE comparisons
2145+
if (
2146+
rootColumnSchema?.type === "JSON" &&
2147+
this.queryHasGroupBy &&
2148+
!this.isInWhereComparisonContext()
2149+
) {
21352150
// Add .:String type hint for JSON subfield access
21362151
result = `${result}.:String`;
21372152
}

0 commit comments

Comments
 (0)