Skip to content

Commit 311c478

Browse files
authored
fix: keep spaces around - operator in dialects with dashed identifiers (#953)
With `denseOperators: true` on BigQuery, a subtraction like `a - b` comes out as `a-b`. That's not just ugly: BigQuery allows dashes inside identifiers, so the densed output re-parses as a single identifier `a-b`, and `format(format(sql))` no longer matches `format(sql)`. `a - foo(y)` is worse — it becomes `a-foo (y)`, turning the expression into an identifier followed by a call. The fix is to leave the `-` operator spaced in dialects that allow dashed identifiers (currently just BigQuery). I derived that flag from the existing `identChars.dashes` tokenizer option so there's a single source of truth. Numeric cases like `1 - 2` were already safe (the `-` is parsed into the literal), so this only affects the identifier case. The shared dense-operator test in `operators.ts` was actually asserting the broken `foo-bar` output for BigQuery; I scoped that to the dashed-identifier dialects and added a BigQuery regression test. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Fixed formatting of the `-` operator in dialects that support dashes in identifiers, such as BigQuery. When dense operator formatting is enabled, spaces around the `-` operator are now properly preserved to prevent expressions from being incorrectly merged into dashed identifiers. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents f3707ba + 68e8f71 commit 311c478

4 files changed

Lines changed: 43 additions & 7 deletions

File tree

src/dialect.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,15 +34,17 @@ export const createDialect = (options: DialectOptions): Dialect => {
3434

3535
const dialectFromOptions = (dialectOptions: DialectOptions): Dialect => ({
3636
tokenizer: new Tokenizer(dialectOptions.tokenizerOptions, dialectOptions.name),
37-
formatOptions: processDialectFormatOptions(dialectOptions.formatOptions),
37+
formatOptions: processDialectFormatOptions(dialectOptions),
3838
});
3939

40-
const processDialectFormatOptions = (
41-
options: DialectFormatOptions
42-
): ProcessedDialectFormatOptions => ({
40+
const processDialectFormatOptions = ({
41+
tokenizerOptions,
42+
formatOptions: options,
43+
}: DialectOptions): ProcessedDialectFormatOptions => ({
4344
alwaysDenseOperators: options.alwaysDenseOperators || [],
4445
onelineClauses: Object.fromEntries(options.onelineClauses.map(name => [name, true])),
4546
tabularOnelineClauses: Object.fromEntries(
4647
(options.tabularOnelineClauses ?? options.onelineClauses).map(name => [name, true])
4748
),
49+
identifierDashes: Boolean(tokenizerOptions.identChars?.dashes),
4850
});

src/formatter/ExpressionFormatter.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,10 @@ export interface ProcessedDialectFormatOptions {
6060
alwaysDenseOperators: string[];
6161
onelineClauses: Record<string, boolean>;
6262
tabularOnelineClauses: Record<string, boolean>;
63+
// True when the dialect allows dashes inside identifiers (e.g. BigQuery).
64+
// In such dialects the "-" operator must keep its surrounding spaces,
65+
// otherwise "a - b" densed to "a-b" would re-parse as a single identifier.
66+
identifierDashes: boolean;
6367
}
6468

6569
/** Formats a generic SQL expression */
@@ -330,7 +334,12 @@ export default class ExpressionFormatter {
330334
}
331335

332336
private formatOperator({ text }: OperatorNode) {
333-
if (this.cfg.denseOperators || this.dialectCfg.alwaysDenseOperators.includes(text)) {
337+
// In dialects that allow dashes inside identifiers (e.g. BigQuery) the "-"
338+
// operator must keep its surrounding spaces. Densing "a - b" into "a-b"
339+
// would otherwise re-parse as a single dashed identifier.
340+
if (text === '-' && this.dialectCfg.identifierDashes) {
341+
this.layout.add(text, WS.SPACE);
342+
} else if (this.cfg.denseOperators || this.dialectCfg.alwaysDenseOperators.includes(text)) {
334343
this.layout.add(WS.NO_SPACE, text);
335344
} else if (text === ':') {
336345
this.layout.add(WS.NO_SPACE, text, WS.SPACE);

test/bigquery.test.ts

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,10 @@ describe('BigQueryFormatter', () => {
5858
'EXCEPT DISTINCT',
5959
'INTERSECT DISTINCT',
6060
]);
61-
supportsOperators(format, ['&', '|', '^', '~', '>>', '<<', '||', '=>'], { any: true });
61+
supportsOperators(format, ['&', '|', '^', '~', '>>', '<<', '||', '=>'], {
62+
any: true,
63+
identifierDashes: true,
64+
});
6265
supportsIsDistinctFrom(format);
6366
supportsParams(format, { positional: true, named: ['@'], quoted: ['@``'] });
6467
supportsWindow(format);
@@ -80,6 +83,19 @@ describe('BigQueryFormatter', () => {
8083
`);
8184
});
8285

86+
// Because dashes are allowed inside identifiers, densing the "-" operator
87+
// would glue its operands into a single identifier ("a - b" -> "a-b"),
88+
// changing the meaning of the query. So denseOperators must keep it spaced.
89+
it('keeps spaces around the - operator in dense mode', () => {
90+
expect(format('SELECT a - b, x - foo(y)\nFROM t', { denseOperators: true })).toBe(dedent`
91+
SELECT
92+
a - b,
93+
x - foo (y)
94+
FROM
95+
t
96+
`);
97+
});
98+
8399
it('supports @@variables', () => {
84100
expect(format('SELECT @@error.message, @@time_zone')).toBe(dedent`
85101
SELECT

test/features/operators.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@ import { FormatFn } from '../../src/sqlFormatter.js';
55
type OperatorsConfig = {
66
logicalOperators?: string[];
77
any?: boolean;
8+
// True for dialects that allow dashes inside identifiers (e.g. BigQuery),
9+
// where the "-" operator must keep its surrounding spaces even in dense mode.
10+
identifierDashes?: boolean;
811
};
912

1013
export default function supportsOperators(
@@ -27,7 +30,13 @@ export default function supportsOperators(
2730

2831
operators.forEach(op => {
2932
it(`supports ${op} operator in dense mode`, () => {
30-
expect(format(`foo ${op} bar`, { denseOperators: true })).toBe(`foo${op}bar`);
33+
// In dialects with dashed identifiers, "foo-bar" would re-parse as a
34+
// single identifier, so the "-" operator keeps its surrounding spaces.
35+
if (op === '-' && cfg.identifierDashes) {
36+
expect(format(`foo ${op} bar`, { denseOperators: true })).toBe(`foo ${op} bar`);
37+
} else {
38+
expect(format(`foo ${op} bar`, { denseOperators: true })).toBe(`foo${op}bar`);
39+
}
3140
});
3241
});
3342

0 commit comments

Comments
 (0)