Skip to content

Commit b8147c9

Browse files
committed
fix: address code review findings — Oracle TRUNC, dialect-aware quoting, query+partition guard
- Oracle day granularity: 'DDD' (day-of-year) → 'DD' (day-of-month) - Add `quoteIdentForDialect()` helper: MySQL/ClickHouse use backticks, TSQL/Fabric use brackets, others use ANSI double-quotes - `buildPartitionDiscoverySQL` and `buildPartitionWhereClause` now use dialect-aware quoting instead of hardcoded double-quotes - `runPartitionedDiff` rejects SQL queries as source/target with a clear error — partitioning requires table names to discover column values
1 parent e41e5a0 commit b8147c9

1 file changed

Lines changed: 34 additions & 6 deletions

File tree

  • packages/opencode/src/altimate/native/connections

packages/opencode/src/altimate/native/connections/data-diff.ts

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,24 @@ const MAX_STEPS = 200
403403
// Partition support
404404
// ---------------------------------------------------------------------------
405405

406+
/**
407+
* Quote a SQL identifier using the correct delimiter for the dialect.
408+
*/
409+
function quoteIdentForDialect(identifier: string, dialect: string): string {
410+
switch (dialect) {
411+
case "mysql":
412+
case "mariadb":
413+
case "clickhouse":
414+
return `\`${identifier.replace(/`/g, "``")}\``
415+
case "tsql":
416+
case "fabric":
417+
return `[${identifier.replace(/\]/g, "]]")}]`
418+
default:
419+
// ANSI SQL: Postgres, Snowflake, BigQuery, DuckDB, Oracle, Redshift, etc.
420+
return `"${identifier.replace(/"/g, '""')}"`
421+
}
422+
}
423+
406424
/**
407425
* Build a DATE_TRUNC expression appropriate for the warehouse dialect.
408426
*/
@@ -421,7 +439,7 @@ function dateTruncExpr(granularity: string, column: string, dialect: string): st
421439
case "oracle": {
422440
// Oracle uses TRUNC() with format models — 'WEEK' is invalid, use 'IW' for ISO week
423441
const oracleFmt: Record<string, string> = {
424-
day: "DDD",
442+
day: "DD",
425443
week: "IW",
426444
month: "MM",
427445
year: "YYYY",
@@ -465,15 +483,16 @@ function buildPartitionDiscoverySQL(
465483
): string {
466484
const where = whereClause ? `WHERE ${whereClause}` : ""
467485
const mode = partitionMode(granularity, bucketSize)
486+
const quotedCol = quoteIdentForDialect(partitionColumn, dialect)
468487

469488
let expr: string
470489
if (mode === "numeric") {
471-
expr = `FLOOR(${partitionColumn} / ${bucketSize}) * ${bucketSize}`
490+
expr = `FLOOR(${quotedCol} / ${bucketSize}) * ${bucketSize}`
472491
} else if (mode === "date") {
473-
expr = dateTruncExpr(granularity!, partitionColumn, dialect)
492+
expr = dateTruncExpr(granularity!, quotedCol, dialect)
474493
} else {
475494
// categorical — raw distinct values, no transformation
476-
expr = partitionColumn
495+
expr = quotedCol
477496
}
478497

479498
return `SELECT DISTINCT ${expr} AS _p FROM ${table} ${where} ORDER BY _p`
@@ -490,8 +509,8 @@ function buildPartitionWhereClause(
490509
dialect: string,
491510
): string {
492511
const mode = partitionMode(granularity, bucketSize)
493-
// Quote the column identifier to handle special characters and reserved words
494-
const quotedCol = `"${partitionColumn.replace(/"/g, '""')}"`
512+
// Quote the column identifier using dialect-appropriate delimiters
513+
const quotedCol = quoteIdentForDialect(partitionColumn, dialect)
495514

496515
if (mode === "numeric") {
497516
const lo = Number(partitionValue)
@@ -592,6 +611,15 @@ function mergeOutcomes(accumulated: unknown, next: unknown): unknown {
592611
* then aggregate results.
593612
*/
594613
async function runPartitionedDiff(params: DataDiffParams): Promise<DataDiffResult> {
614+
// Partitioned diff requires table names — can't partition a SQL query by column
615+
if (isQuery(params.source) || isQuery(params.target)) {
616+
return {
617+
success: false,
618+
error: "partition_column cannot be used when source or target is a SQL query. Use table names instead, or remove partition_column.",
619+
steps: 0,
620+
}
621+
}
622+
595623
const resolveDialect = (warehouse: string | undefined): string => {
596624
if (warehouse) {
597625
const cfg = Registry.getConfig(warehouse)

0 commit comments

Comments
 (0)