Skip to content

fix: only split a query at a standalone SETTINGS keyword #3216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kodiakhq merged 5 commits into from
Sep 29, 2026
Merged

fix: only split a query at a standalone SETTINGS keyword #3216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e7d9860
fix: only split a query at a standalone SETTINGS keyword
breken-ai Sep 25, 2026
542bb85
fix: skip comments and dotted settings paths when finding SETTINGS
breken-ai Sep 25, 2026
ff0d6bd
test: Add more extractSettingsClauseFromEnd tests
pulpdrew Sep 29, 2026
62a13f7
Merge remote-tracking branch 'origin/main' into ai/fix-settings-keyword
pulpdrew Sep 29, 2026
25609fb
Merge branch 'main' into ai/fix-settings-keyword
pulpdrew Sep 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .changeset/settings-keyword-in-identifiers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
'@hyperdx/common-utils': patch
'@hyperdx/api': patch
'@hyperdx/app': patch
---

fix: only split a query at a standalone SETTINGS keyword

`extractSettingsClauseFromEnd` cut the query at the first "settings" anywhere in
it, including inside a string or an identifier. A multi-series metric chart with
a metric such as `app.settings.reloads` produced SQL with an unterminated
string, and a SQL filter on a column such as `AppSettings` was rewritten to
reference `App`. The keyword now has to stand alone outside quotes.
18 changes: 18 additions & 0 deletions packages/common-utils/src/__tests__/renderChartConfig.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4313,6 +4313,24 @@ describe('renderChartConfig', () => {
expect(sql).toContain('GROUP BY ALL ORDER BY `__hdx_time_bucket`');
});

it('keeps a metric name containing "settings" intact in its branch', async () => {
const generatedSql = await renderChartConfig(
{
...baseMultiSeriesConfig,
select: [
gaugeSelect('app.settings.reloads'),
gaugeSelect('metric.beta'),
],
},
mockMetadata,
querySettings,
);
const sql = parameterizedQueryToSql(generatedSql);

expect(sql).toContain("MetricName = 'app.settings.reloads'");
expect(sql.match(/SETTINGS/g)).toHaveLength(1);
});

it('renders a metric ratio as a SQL-side division', async () => {
const generatedSql = await renderChartConfig(
{
Expand Down
79 changes: 79 additions & 0 deletions packages/common-utils/src/__tests__/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2766,6 +2766,85 @@ describe('utils', () => {
// This test case illustrates that subsequent clauses will also be extracted.
settingsClause: 'SETTINGS opt = 1, cast = 1 FORMAT json',
},
{
label: 'settings inside an identifier',
sql: 'SELECT * FROM table WHERE AppSettings = 1 SETTINGS opt = 1',
withoutSettingsClause: 'SELECT * FROM table WHERE AppSettings = 1',
settingsClause: 'SETTINGS opt = 1',
},
{
label: 'settings inside a string literal',
sql: "SELECT * FROM table WHERE MetricName = 'app.settings.reloads'",
withoutSettingsClause:
"SELECT * FROM table WHERE MetricName = 'app.settings.reloads'",
settingsClause: undefined,
},
{
label: 'settings as a dotted path or map column',
sql: "SELECT * FROM table WHERE LogAttributes.settings = 'x' AND settings['k'] = 'y'",
withoutSettingsClause:
"SELECT * FROM table WHERE LogAttributes.settings = 'x' AND settings['k'] = 'y'",
settingsClause: undefined,
},
{
label: 'apostrophe in a comment before SETTINGS',
sql: "SELECT * FROM table -- don't count retries\nWHERE a = 1 SETTINGS max_threads = 1",
withoutSettingsClause:
"SELECT * FROM table -- don't count retries\nWHERE a = 1",
settingsClause: 'SETTINGS max_threads = 1',
},
{
label: 'apostrophe in a block comment before SETTINGS',
sql: "SELECT * FROM table /* don't count retries */ WHERE a = 1 SETTINGS max_threads = 1",
withoutSettingsClause:
"SELECT * FROM table /* don't count retries */ WHERE a = 1",
settingsClause: 'SETTINGS max_threads = 1',
},
{
label: 'settings inside a comment',
sql: 'SELECT * FROM table -- SETTINGS in a comment\nWHERE a = 1',
withoutSettingsClause:
'SELECT * FROM table -- SETTINGS in a comment\nWHERE a = 1',
settingsClause: undefined,
},
{
label: 'escaped quote in a string literal before settings',
sql: "SELECT * FROM table WHERE a = 'it\\'s settings' SETTINGS max_threads = 1",
withoutSettingsClause:
"SELECT * FROM table WHERE a = 'it\\'s settings'",
settingsClause: 'SETTINGS max_threads = 1',
},
{
label: 'doubled quote in a string literal before settings',
sql: "SELECT * FROM table WHERE a = 'it''s settings' SETTINGS max_threads = 1",
withoutSettingsClause: "SELECT * FROM table WHERE a = 'it''s settings'",
settingsClause: 'SETTINGS max_threads = 1',
},
{
label: 'settings as a quoted identifier',
sql: 'SELECT `settings`, "settings" FROM table SETTINGS max_threads = 1',
withoutSettingsClause: 'SELECT `settings`, "settings" FROM table',
settingsClause: 'SETTINGS max_threads = 1',
},
{
label: 'settings as a table name',
sql: 'SELECT name FROM system.settings WHERE changed SETTINGS max_threads = 1',
withoutSettingsClause: 'SELECT name FROM system.settings WHERE changed',
settingsClause: 'SETTINGS max_threads = 1',
},
{
label: 'lowercase keyword followed by a newline',
sql: 'SELECT * FROM table WHERE a = 1 settings\n max_threads = 1',
withoutSettingsClause: 'SELECT * FROM table WHERE a = 1',
settingsClause: 'settings\n max_threads = 1',
},
{
label: 'string value inside the SETTINGS clause',
sql: "SELECT * FROM table SETTINGS short_circuit_function_evaluation = 'force_enable'",
withoutSettingsClause: 'SELECT * FROM table',
settingsClause:
"SETTINGS short_circuit_function_evaluation = 'force_enable'",
},
])(
'Extracts SETTINGS clause from: "$label" query',
({ sql, settingsClause, withoutSettingsClause }) => {
Expand Down
40 changes: 39 additions & 1 deletion packages/common-utils/src/core/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1209,6 +1209,44 @@ export function isDateRangeEqual(range1: [Date, Date], range2: [Date, Date]) {
);
}

/**
* Index of the first standalone SETTINGS keyword, or -1. Occurrences inside
* quoted strings, comments or identifiers (e.g. `'app.settings.reloads'`,
* `AppSettings`, `LogAttributes.settings`) are not the clause and must not
* split the query.
*/
function findSettingsKeyword(sql: string): number {

@github-actions github-actions Bot Sep 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — A second quote-aware keyword scanner, and this one skips SQL comments

extractOuterSelectProjection in packages/common-utils/src/clickhouse/index.ts:855-937 already does this: its matchesKeywordAt/isWordChar scanner tracks quotes and word boundaries and also skips -- and /* */ comments. The new findSettingsKeyword has no comment handling. So an apostrophe inside a comment (-- don't count retries\n... SETTINGS max_threads=1) opens a fake string, and the real SETTINGS is never found. chSqlToAliasMap and fastifySQL then pass SETTINGS to SQLParser, which fails to parse it. On main, plain indexOf found that clause. Pull the scanner out into a shared keyword finder with comment skipping, and use it in both places.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

@breken-ai breken-ai Sep 25, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 542bb85. The scanner now skips -- and /* */ comments. A new extractSettingsClauseFromEnd case ("apostrophe in a comment before SETTINGS") fails on e7d9860 and passes now. I didn't reuse extractOuterSelectProjection's scanner here, because that would touch clickhouse/index.ts, which this PR doesn't otherwise change.

@github-actions github-actions Bot Sep 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — findSettingsKeyword copies an existing scanner in common-utils and leaves out its parenthesis-depth tracking

extractOuterSelectProjection in packages/common-utils/src/clickhouse/index.ts:853 already skips quotes, backticks, -- and /* */ comments, and matches keywords only at word boundaries (isWordChar/matchesKeywordAt). It also tracks parenDepth, which the new function doesn't. Here is where that matters: renderWith (renderChartConfig.ts:1800) renders a chartConfig CTE with the same querySettings, so the SQL becomes WITH x AS (… SETTINGS k = v) SELECT … SETTINGS k = v. The new scanner returns the SETTINGS inside the CTE, and chSqlToAliasMap then gets back an unbalanced prefix. Move the tokenizer into a shared helper that both functions use, and only accept SETTINGS at parenDepth === 0. That fits the 'from the end' contract in the function's doc comment, and it satisfies the repo's REQUIRED DRY/grep-before-adding-a-helper rule.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

const isWordChar = (c: string) => /\w/.test(c);
const nextNonSpace = (from: number) => sql.slice(from).trimStart().charAt(0);
let quote: string | undefined;
for (let i = 0; i < sql.length; i++) {
const c = sql.charAt(i);
if (quote) {
if (c === '\\') i++;
else if (c === quote) quote = undefined;
} else if (c === "'" || c === '"' || c === '`') {
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Resolved
quote = c;
} else if (c === '-' && sql.charAt(i + 1) === '-') {
const lineEnd = sql.indexOf('\n', i + 2);
if (lineEnd === -1) break;
i = lineEnd;
} else if (c === '/' && sql.charAt(i + 1) === '*') {
const blockEnd = sql.indexOf('*/', i + 2);
if (blockEnd === -1) break;
i = blockEnd + 1;
} else if (
sql.substring(i, i + 8).toUpperCase() === 'SETTINGS' &&

@github-actions github-actions Bot Sep 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — A dotted path or map column named settings still splits the query

The word-boundary check uses \w, and . is not a word character. So a filter like LogAttributes.settings = 'x', a JSON path ResourceAttributes.settings.mode, or a map column settings['k'] still matches as a standalone SETTINGS. fastifySQL then truncates it to LogAttributes.. This is the same class of bug the PR fixes. Only treat a match as the clause if the previous non-space character is not . and the next non-space character is not . or [, and add a test case for Attributes.settings.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

@breken-ai breken-ai Sep 25, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 542bb85. A match is skipped when the previous non-space character is . or the next one is . or [. A new "settings as a dotted path or map column" case (LogAttributes.settings = 'x' AND settings['k'] = 'y') fails on e7d9860 and passes now. common-utils unit suite: 2655 pass. The 2 failures also happen on unmodified main (a docs file missing from my sparse checkout, and extractColumnReferencesFromKey array access). tsc is clean. eslint stays at 80 warnings.

!isWordChar(sql.charAt(i - 1)) &&
!isWordChar(sql.charAt(i + 8)) &&
sql.slice(0, i).trimEnd().slice(-1) !== '.' &&
!['.', '['].includes(nextNonSpace(i + 8))
) {
return i;
}
}
return -1;
}

/*
This function extracts the SETTINGS clause from the end(!) of the sql string.
*/
Expand All @@ -1219,7 +1257,7 @@ export function extractSettingsClauseFromEnd(
? sqlInput.trim().slice(0, -1)
: sqlInput.trim();

const settingsIndex = sql.toUpperCase().indexOf('SETTINGS');
const settingsIndex = findSettingsKeyword(sql);

if (settingsIndex === -1) {
return [sql, undefined] as const;
Expand Down
Loading