Skip to content

Commit a684ce5

Browse files
committed
Fix for subpath access to JSON fields
1 parent 34749d5 commit a684ce5

File tree

2 files changed

+93
-4
lines changed

2 files changed

+93
-4
lines changed

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,58 @@ describe("ClickHousePrinter", () => {
545545
// Should group by the raw column
546546
expect(sql).toContain("GROUP BY error");
547547
});
548+
549+
it("should cast JSON subfield to String with underscore alias in SELECT and GROUP BY", () => {
550+
const ctx = createJsonContext();
551+
const { sql, columns } = printQuery(
552+
"SELECT error.data.name, count() AS error_count FROM runs GROUP BY error.data.name",
553+
ctx
554+
);
555+
556+
// SELECT should use .:String type hint with underscore alias
557+
expect(sql).toContain("error.data.name.:String AS error_data_name");
558+
// GROUP BY should use .:String type hint (no alias needed)
559+
expect(sql).toContain("GROUP BY error.data.name.:String");
560+
561+
// Column metadata should have the underscore alias name
562+
expect(columns).toContainEqual(
563+
expect.objectContaining({ name: "error_data_name", type: "String" })
564+
);
565+
expect(columns).toContainEqual(
566+
expect.objectContaining({ name: "error_count", type: "UInt64" })
567+
);
568+
});
569+
570+
it("should cast JSON subfield to String with multiple fields and underscore aliases", () => {
571+
const ctx = createJsonContext();
572+
const { sql, columns } = printQuery(
573+
"SELECT error.name, error.message, count() AS cnt FROM runs GROUP BY error.name, error.message",
574+
ctx
575+
);
576+
577+
// SELECT should have type hints with underscore aliases
578+
expect(sql).toContain("error.name.:String AS error_name");
579+
expect(sql).toContain("error.message.:String AS error_message");
580+
// GROUP BY should have type hints
581+
expect(sql).toContain("GROUP BY error.name.:String, error.message.:String");
582+
583+
// Column metadata should have correct names
584+
expect(columns).toContainEqual(
585+
expect.objectContaining({ name: "error_name", type: "String" })
586+
);
587+
expect(columns).toContainEqual(
588+
expect.objectContaining({ name: "error_message", type: "String" })
589+
);
590+
});
591+
592+
it("should not cast non-JSON columns in GROUP BY", () => {
593+
const ctx = createJsonContext();
594+
const { sql } = printQuery("SELECT status, count() AS cnt FROM runs GROUP BY status", ctx);
595+
596+
// Regular columns should not have type hints
597+
expect(sql).toContain("GROUP BY status");
598+
expect(sql).not.toContain(".:String");
599+
});
548600
});
549601

550602
describe("ORDER BY clauses", () => {

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

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -497,6 +497,8 @@ export class ClickHousePrinter {
497497

498498
// Extract output name and source column before visiting
499499
const { outputName, sourceColumn, inferredType } = this.analyzeSelectColumn(col);
500+
// effectiveOutputName may be overridden for JSON subfields
501+
let effectiveOutputName = outputName;
500502

501503
// Check if this is a bare Field (not wrapped in Alias)
502504
let sqlResult: string;
@@ -513,9 +515,19 @@ export class ClickHousePrinter {
513515
// Visit the field to get the ClickHouse SQL
514516
const visited = this.visit(col);
515517

518+
// Check if this is a JSON subfield access (will have .:String type hint)
519+
// If so, add an alias to preserve the nice column name (dots → underscores)
520+
const isJsonSubfield = this.isJsonSubfieldAccess(field.chain);
521+
if (isJsonSubfield) {
522+
// Build the alias using underscores (e.g., "error_data_name")
523+
const aliasName = field.chain.filter((p): p is string => typeof p === "string").join("_");
524+
sqlResult = `${visited} AS ${this.printIdentifier(aliasName)}`;
525+
// Override output name for metadata
526+
effectiveOutputName = aliasName;
527+
}
516528
// Check if the column has a different clickhouseName - if so, add an alias
517529
// to ensure results come back with the user-facing name
518-
if (
530+
else if (
519531
outputName &&
520532
sourceColumn?.clickhouseName &&
521533
sourceColumn.clickhouseName !== outputName
@@ -546,9 +558,9 @@ export class ClickHousePrinter {
546558
}
547559

548560
// Collect metadata for top-level queries
549-
if (collectMetadata && outputName) {
561+
if (collectMetadata && effectiveOutputName) {
550562
const metadata: OutputColumnMetadata = {
551-
name: outputName,
563+
name: effectiveOutputName,
552564
type: sourceColumn?.type ?? inferredType ?? "String",
553565
};
554566

@@ -1882,7 +1894,21 @@ export class ClickHousePrinter {
18821894
const resolvedChain = this.resolveFieldChain(node.chain);
18831895

18841896
// Print each chain element
1885-
return resolvedChain.map((part) => this.printIdentifierOrIndex(part)).join(".");
1897+
let result = resolvedChain.map((part) => this.printIdentifierOrIndex(part)).join(".");
1898+
1899+
// For JSON column subfield access (e.g., error.data.name), add .:String type hint
1900+
// This is required because ClickHouse's Dynamic/Variant types are not allowed in
1901+
// GROUP BY without type casting, and SELECT/GROUP BY expressions must match
1902+
if (resolvedChain.length > 1) {
1903+
// Check if the root column (first part) is a JSON column
1904+
const rootColumnSchema = this.resolveFieldToColumnSchema([node.chain[0]]);
1905+
if (rootColumnSchema?.type === "JSON") {
1906+
// Add .:String type hint for JSON subfield access
1907+
result = `${result}.:String`;
1908+
}
1909+
}
1910+
1911+
return result;
18861912
}
18871913

18881914
/**
@@ -1934,6 +1960,17 @@ export class ClickHousePrinter {
19341960
return chain;
19351961
}
19361962

1963+
/**
1964+
* Check if a field chain represents JSON subfield access (e.g., error.data.name)
1965+
* Returns true if the root column is JSON type and there are additional path parts
1966+
*/
1967+
private isJsonSubfieldAccess(chain: Array<string | number>): boolean {
1968+
if (chain.length <= 1) return false;
1969+
1970+
const rootColumnSchema = this.resolveFieldToColumnSchema([chain[0]]);
1971+
return rootColumnSchema?.type === "JSON";
1972+
}
1973+
19371974
/**
19381975
* Resolve a field chain to its column schema (if it references a known column)
19391976
*/

0 commit comments

Comments
 (0)