Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
59 changes: 45 additions & 14 deletions src/parser/grammar.ne
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,20 @@ interface CommentAttachments {
trailing?: CommentNode[];
}

interface ExpressionList {
previous?: ExpressionList;
value: AstNode;
}

const materializeExpressionList = (expressions?: ExpressionList): AstNode[] => {
const result: AstNode[] = [];
for (let current = expressions; current; current = current.previous) {
result.push(current.value);
}
result.reverse();
return result;
};

const addComments = (node: AstNode, { leading, trailing }: CommentAttachments): AstNode => {
if (leading?.length) {
node = { ...node, leadingComments: leading };
Expand Down Expand Up @@ -83,14 +97,24 @@ main -> statement:* {%
statement -> expressions_or_clauses (%DELIMITER | %EOF) {%
([children, [delimiter]]) => ({
type: NodeType.statement,
children,
children: materializeExpressionList(children),
hasSemicolon: delimiter.type === TokenType.DELIMITER,
})
%}

# To avoid ambiguity, plain expressions can only come before clauses
expressions_or_clauses -> free_form_sql:* clause:* {%
([expressions, clauses]) => [...expressions, ...clauses]
# For performance, keep both in one persistent list until the surrounding rule is complete.
expressions_or_clauses -> expression_list {% id %}
expressions_or_clauses -> expressions_or_clauses clause {%
([previous, value]) => ({ previous, value })
%}

# Avoid free_form_sql:*: Nearley implements it with array concatenation, which
# copies every shared prefix and makes long expression lists quadratic.
# Nearley's null matches no input; undefined represents an empty linked list.
expression_list -> null {% () => undefined %}
expression_list -> expression_list free_form_sql {%
([previous, value]) => ({ previous, value })
%}

clause ->
Expand Down Expand Up @@ -119,11 +143,14 @@ limit_clause -> %LIMIT _ expression_chain_ (%COMMA free_form_sql:+):? {%
}
%}

select_clause -> %RESERVED_SELECT (all_columns_asterisk free_form_sql:* | asteriskless_free_form_sql free_form_sql:*) {%
select_clause -> %RESERVED_SELECT (all_columns_asterisk expression_list | asteriskless_free_form_sql expression_list) {%
([nameToken, [exp, expressions]]) => ({
type: NodeType.clause,
nameKw: toKeywordNode(nameToken),
children: [exp, ...expressions],
// Nearley completes every prefix; only materialize the surviving clause's children.
get children(): AstNode[] {
return [exp, ...materializeExpressionList(expressions)];
Comment on lines +151 to +152

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of this wrapping of children property into a getter function?

It looks like some an additional performance optimization. But not sure it's really adding anything. At least when I removed these, the code still seemed to perform similarly.

If this is a separate unrelated optimization, then please make a separate PR with it. If it's tightly related to the current optimization and really needed for it, please provide some explanation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah this does help in cases like SELECT column_1, column_2, column_3, ..., column_1000 FROM my_table or SELECT * FROM my_table WHERE id = 1 OR id = 2 OR id = 3 ... OR id = 1000;. Nearley creates an intermediate clause for every prefix, so eagerly constructing children repeatedly copies the entire prefix and makes this quadratic.

The test from eb187fe mostly demonstrates this case. It does "pass" without this change because peak/final memory is lowered (by only the linkedlist change), but it still takes ~20x longer without this (using quadratic total memory, even if not peak memory)

I could split this into a separate follow-up PR if that helps, was just broadly tackling the several related issues of "quadratic costs"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK. Thanks for the explanation. Makes sense now.

},
})
%}
select_clause -> %RESERVED_SELECT {%
Expand All @@ -138,19 +165,23 @@ all_columns_asterisk -> %ASTERISK {%
() => ({ type: NodeType.all_columns_asterisk })
%}

other_clause -> %RESERVED_CLAUSE free_form_sql:* {%
other_clause -> %RESERVED_CLAUSE expression_list {%
([nameToken, children]) => ({
type: NodeType.clause,
nameKw: toKeywordNode(nameToken),
children,
get children(): AstNode[] {
return materializeExpressionList(children);
},
})
%}

set_operation -> %RESERVED_SET_OPERATION free_form_sql:* {%
set_operation -> %RESERVED_SET_OPERATION expression_list {%
([nameToken, children]) => ({
type: NodeType.set_operation,
nameKw: toKeywordNode(nameToken),
children,
get children(): AstNode[] {
return materializeExpressionList(children);
},
})
%}

Expand Down Expand Up @@ -232,25 +263,25 @@ function_call -> %RESERVED_FUNCTION_NAME _ parenthesis {%
parenthesis -> "(" expressions_or_clauses ")" {%
([open, children, close]) => ({
type: NodeType.parenthesis,
children: children,
children: materializeExpressionList(children),
openParen: "(",
closeParen: ")",
})
%}

curly_braces -> "{" free_form_sql:* "}" {%
curly_braces -> "{" expression_list "}" {%
([open, children, close]) => ({
type: NodeType.parenthesis,
children: children,
children: materializeExpressionList(children),
openParen: "{",
closeParen: "}",
})
%}

square_brackets -> "[" free_form_sql:* "]" {%
square_brackets -> "[" expression_list "]" {%
([open, children, close]) => ({
type: NodeType.parenthesis,
children: children,
children: materializeExpressionList(children),
openParen: "[",
closeParen: "]",
})
Expand Down
29 changes: 28 additions & 1 deletion test/perftest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ describe('Performance test', () => {
});

// Issue #840
it.skip('should use less than 100 MB of additional memory to format ~100 KB of SQL', () => {
it('should use less than 100 MB of additional memory to format ~100 KB of SQL', () => {
// Long list of values
const values = Array(10000).fill('myid');
const sql = `SELECT ${values.join(', ')}`;
Expand All @@ -21,6 +21,33 @@ describe('Performance test', () => {

expect(memoryUsageInMB()).toBeLessThan(BASELINE + 100);
});

it('formats all 10,000 values in a parenthesized IN list', () => {
const values = Array(10000).fill('myid');
const sql = `SELECT * FROM my_table WHERE col IN (${values.join(', ')})`;

const formatted = format(sql, { language: 'sql' });

expect(formatted.match(/\bmyid\b/g)).toHaveLength(values.length);
});

it('formats a parenthesized OR chain with 5,000 conditions', () => {
const conditions = Array(5000).fill('col = myid');
const sql = `SELECT * FROM my_table WHERE (${conditions.join(' OR ')})`;

const formatted = format(sql, { language: 'sql' });

expect(formatted.match(/\bOR\b/g)).toHaveLength(conditions.length - 1);
});

it('formats an unparenthesized OR chain with 5,000 conditions', () => {
const conditions = Array(5000).fill('col = myid');
const sql = `SELECT * FROM my_table WHERE ${conditions.join(' OR ')}`;

const formatted = format(sql, { language: 'sql' });

expect(formatted.match(/\bOR\b/g)).toHaveLength(conditions.length - 1);
});
});

function memoryUsageInMB() {
Expand Down
Loading