Fix parser performance rebased - #8
Merged
nikita-volkov merged 10 commits intoJul 26, 2026
Merged
Conversation
Closed
LennartSpitzner
force-pushed
the
fix-parser-performance-rebased
branch
from
February 10, 2023 15:39
ae3b1fe to
a881e4c
Compare
Owner
|
Hey! Thanks and sorry for not noticing it sooner. Had the notifications disabled on this repository for some reason |
nikita-volkov
left a comment
nikita-volkov
left a comment
Owner
There was a problem hiding this comment.
Since this is a rather drastic change, can we first establish the essence of the issue and why you chose the approach you've taken?
# Conflicts: # library/PostgresqlSyntax/Parsing.hs
The tests expect one of the two possible (ambiguous) parses (specifically, they test round-tripping of generated parse-trees that only contain this parse, not the equivalent other). So we transform the parser output to only contain this parse.
LennartSpitzner
force-pushed
the
fix-parser-performance-rebased
branch
from
February 28, 2023 14:12
8a6d4db to
7f14662
Compare
Contributor
Author
I am not sure what you are asking for. I have provided a test-case, plus a long documentation about changes and their reasoning. Specifically, to reproduce the issue see the code below: -- inserted at top of `PostgresqlSyntax.Parsing`
test :: IO ()
test = do
let input = "((((((( COALESCE (mytable.oiuqweu_puiwrip_eoprdc, 0) + COALESCE (mytable.insdad_eoprdc, 0) + COALESCE (mytable.basdnasd_rates_eoprdc, 0) + COALESCE (mytable.poadpod_tpio_eoprdc, 0) + COALESCE (mytable.mkadldod_puiwrip_eoprdc, 0) + COALESCE (mytable.woidp_eoprdc, 0) + COALESCE (mytable.poiqwehda_eoprdc, 0) + COALESCE (mytable.htaing_eoprdc, 0) + COALESCE (mytable.clingd_eoprdc, 0) + COALESCE (mytable.lkjaldjj_eoprdc, 0) + COALESCE (mytable.dopasdop_eoprdc, 0) + COALESCE (mytable.thotd_idiaspdoid_eoprdc, 0) ) - ( COALESCE (mytable.poadpod_tpio, 0) + COALESCE (mytable.mkadldod_puiwrip, 0) + COALESCE (mytable.insdad_poaisd, 0) + COALESCE (mytable.basdnasd_rates_poaisd, 0) + COALESCE (mytable.oiuqweu_puiwrip_poaisd, 0) + COALESCE (mytable.lkjfjf_dopasdop_cost_iuasudd, 0) + COALESCE (mytable.lkjfjf_htaing_iuasudd, 0) + COALESCE (mytable.prepaud_dopasdop_cost_poaisd, 0) + COALESCE (mytable.prepaud_mkadldod_puiwrip_poaisd, 0) + COALESCE (mytable.prepaud_woidp_poaisd, 0) + COALESCE (mytable.prepaud_poiqwehda_poaisd, 0) + COALESCE (mytable.thotd_idiaspdoid_poaisd, 0) + COALESCE (mytable.lkjfjf_woidp_iuasudd, 0) + COALESCE (mytable.lkjfjf_poiqwehda_iuasudd, 0) + COALESCE (mytable.lkjfjf_clingd_iuasudd, 0) + COALESCE (mytable.lkjfjf_lkjaldjj_iuasudd, 0) + COALESCE (mytable.prepaud_clingd_poaisd, 0) + COALESCE (mytable.prepaud_lkjaldjj_poaisd, 0) + COALESCE (mytable.prepaud_htaing_poaisd, 0) + COALESCE (mytable.prepaud_thotd_idiaspdoid_poaisd, 0) + COALESCE (mytable.lkjfjf_thotd_idiaspdoid_iuasudd, 0) + COALESCE (mytable.lkjfjf_mkadldod_puiwrip_iuasudd, 0) + COALESCE (mytable.dopasdop_eoprdc_poaisd, 0) + COALESCE (mytable.htaing_poaisd, 0) + COALESCE (mytable.clingd_poaisd, 0) + COALESCE (mytable.woidp_poaisd, 0) + COALESCE (mytable.poiqwehda_poaisd, 0) + COALESCE (mytable.lkjaldjj_poaisd, 0))))))))"
case run aExpr input of
Left err -> putStrLn err
Right x -> print xOn current master, this takes .. 90sec to finish on my machine: with this PR this is fixed: Of course using |
There was a problem hiding this comment.
Pull request overview
This PR rebases and continues the work to address exponential-time behavior in expression parsing by refactoring several ambiguous/common-prefix parser alternatives (notably around aExpr/cExpr and nested parenthesized selects).
Changes:
- Refactors
aExpr/cExprparsing to reduce common-prefix backtracking (introducing tail parsers and specialized “no common prefix” variants). - Adds normalization for nested parenthesized selects via
convertNestedParenSelectand applies it in a few expression contexts. - Reimplements
keywordparsing for faster mismatch rejection (first-character guard).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _select <- selectClause | ||
| _select <- case _with of | ||
| Just {} -> selectClause | ||
| Nothing -> selectClauseNoParens |
There was a problem hiding this comment.
sharedSelectNoParens now selects selectClauseNoParens when there is no WITH-clause, which prevents select_no_parens from starting with select_with_parens. This contradicts the grammar reference in the comment above (select_clause sort_clause etc.) and makes valid constructs like (SELECT ...) ORDER BY ... unparseable, because selectStmt cannot fall back to selectWithParens (it wouldn’t consume the trailing ORDER BY). Consider restoring selectClause here and instead introducing a separate “no-leading-parens” variant of selectNoParens only for the contexts that need the performance/ambiguity fix (e.g. inside selectWithParens/cExprTailNoCommonPrefix).
Suggested change
| Nothing -> selectClauseNoParens | |
| Nothing -> selectClause |
Comment on lines
+1268
to
+1274
| -- implicitRow = inParens $ do | ||
| -- a <- wrapToHead aExpr | ||
| -- commaSeparator | ||
| -- b <- exprList | ||
| -- return $ case NonEmpty.consAndUnsnoc a b of | ||
| -- (c, d) -> ImplicitRow c d | ||
|
|
There was a problem hiding this comment.
The old implicitRow implementation is left in the source as a commented-out block. This adds noise in a hot/complex parsing module and makes future maintenance harder; consider removing the commented code and relying on version control for history.
Suggested change
| -- implicitRow = inParens $ do | |
| -- a <- wrapToHead aExpr | |
| -- commaSeparator | |
| -- b <- exprList | |
| -- return $ case NonEmpty.consAndUnsnoc a b of | |
| -- (c, d) -> ImplicitRow c d |
Comment on lines
+1204
to
+1206
| openParenAExpr :: Parser AExpr | ||
| openParenAExpr = char '(' *> space *> aExpr <* endHead | ||
|
|
There was a problem hiding this comment.
openParenAExpr is introduced but doesn’t appear to be referenced anywhere. If it’s not intended as part of the public API (this module exports everything), consider removing it to avoid accumulating unused exported helpers (or at least add a comment indicating intended future use).
Suggested change
| openParenAExpr :: Parser AExpr | |
| openParenAExpr = char '(' *> space *> aExpr <* endHead |
Comment on lines
+2169
to
+2176
| keyword a = parse $ | ||
| Megaparsec.label "keyword" $ do | ||
| _firstChar <- Megaparsec.satisfy Predicate.firstIdentifierChar | ||
| guard (Char.toLower _firstChar == Text.head a) | ||
| _remainder <- Megaparsec.takeWhileP Nothing Predicate.notFirstIdentifierChar | ||
| let r = Text.toLower (Text.cons _firstChar _remainder) | ||
| guard (r == a) | ||
| return r |
There was a problem hiding this comment.
keyword now uses Text.head a, which will throw an exception if called with an empty Text. Since PostgresqlSyntax.Parsing exports everything, this makes keyword a partial (crashable) public helper. Consider handling Text.uncons a/Text.null a explicitly and failing the parser for the empty case instead of crashing.
Suggested change
| keyword a = parse $ | |
| Megaparsec.label "keyword" $ do | |
| _firstChar <- Megaparsec.satisfy Predicate.firstIdentifierChar | |
| guard (Char.toLower _firstChar == Text.head a) | |
| _remainder <- Megaparsec.takeWhileP Nothing Predicate.notFirstIdentifierChar | |
| let r = Text.toLower (Text.cons _firstChar _remainder) | |
| guard (r == a) | |
| return r | |
| keyword a = | |
| case Text.uncons a of | |
| Nothing -> parse $ Megaparsec.label "keyword" $ fail "empty keyword" | |
| Just (expectedFirstChar, _) -> parse $ | |
| Megaparsec.label "keyword" $ do | |
| _firstChar <- Megaparsec.satisfy Predicate.firstIdentifierChar | |
| guard (Char.toLower _firstChar == expectedFirstChar) | |
| _remainder <- Megaparsec.takeWhileP Nothing Predicate.notFirstIdentifierChar | |
| let r = Text.toLower (Text.cons _firstChar _remainder) | |
| guard (r == a) | |
| return r |
Owner
|
Sorry this took so long. Thanks for explaining the issue and providing a fix. I've merged this with some corrections. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased version of the previous PR at #2. Now based on the upstream formatting.
Hey, thanks for maintaining this package! And for considering this non-trivial PR :)
This PR fixes some bad parsing behaviour that leads to exponential time usage.
Testcase
consider the expression
run aExpr inputwhereinputis the above string. Parsing this does terminate, but you definitely start noticing the cpu time, and if you profile you can see some suspiciously large values. We had some larger queries that did not terminate, but with the same underlying problem.Analysis
There are a couple of parsers around
aExpr,customizedAExpr,cExpr,customizedCExprthat have alternatives with common prefixes and with ambiguous parses.customizedAExpr/suffix)aExpris a prefix of bothInParensCExprorImplicitRowCExprcolIdis a prefix of bothFuncCExprandColumnrefCExprOn their own these might be harmless, but once such cases nest they can create a problem. I think the last of the above is just a constant-factor problem (so rather harmless); still, I cleaned these up in the PR because a) it was relatively simple to do b) It is hard to diagnose which common prefixes lead to problems, so it makes debugging the whole thing easier if they get cleaned up.
both should correspond to something like
((SELECT …)).The ambiguity might be known because the hedgehog tests explicitly generate only one of those two options (see
postgresql-syntax/hedgehog-test/Main/Gen.hs
Line 475 in 6caff75
Approach
I follow a simple process: Take any alternative (
asum [..]) where alternatives have common prefixes, remove those alternatives and replace with a single new one that first matches the common prefix, then branches again. As an example, I replacedwith
It is not always this simple, for example if the
inParensappears deeper in the call stack then this gets messier, with multiple newXTailbindings.Notes
I am not sure if I applied
endHeadandwrapHeadcorrectly everywhere. The tests pass, but I am not sure that the tests cover the readability of the error-messages (iiuc the "HeadParser" also has the purpose of helping with those).There are some cases still where the order of the parsers (in
asums) matters. I don't think that is ideal, nor should it be necessary. As I understand it this might be due to committing to some branch (viaendHead) too soon. But it is also harmless given that the tests pass.I added https://github.com/proda-ai/postgresql-syntax/blob/e0450870e35dcbfd4a11d6757bcb54c4bd65cc10/library/PostgresqlSyntax/Parsing.hs#L1288-L1301 after resolving the ambiguity-problem: The tests expect nested
WithParensSelectWithParens, so the parser simply convertsInParensCExpr (CExprAExpr …)to that where necessary. Could instead fix the tests, but maybe the slightly flatter trees for nested selects are worth this cost. I have no idea why exactly the AST types are defined as they are, to be honest.I feel like the code now combines several different approaches to ensure good performance:
customizedCExprapproach to parameterise by some child-parserI am not proud of my addition as it makes the whole logic harder to follow. It might be cleaner make use of continuations, but still that does not improve things much. But I don't see an easy-to-read/maintain approach with good performance ..
At least I only had to touch the "innermost loop" when parsing expressions, so maybe it is worth it.
the fact that this
postgresql-syntax/library/PostgresqlSyntax/Parsing.hs
Line 1025 in 6caff75
aExprinstead ofbasemeans that a) you get a more "left-leaning" output tree but also b) that if parsing fails, you might end up failing again and again for every level of a nestedaExpr. Same forbExpr. I left a note about this in the code. Could be fixed by the same "parse different tree (usingbase), then post-process to restructure the tree" approach that I used for nested selects.might be nice to have performance tests included. There should be some simple trivial expression like
((((((((((a+b))))))))))that where parsing does not terminate on master but is instant with this PR. Might need a bit more nesting. But yeah, perf tests are a bit annoying at times because they can depend on the performance of the system running the tests.