Skip to content
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

Fix bug causing multiple nested nots to parse very slowly #436

Merged
merged 1 commit into from
Sep 9, 2021
Merged

Fix bug causing multiple nested nots to parse very slowly #436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
42 changes: 20 additions & 22 deletions lang/src/org/partiql/lang/syntax/SqlParser.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1118,41 +1118,39 @@ class SqlParser(private val ion: IonSystem) : Parser {
return expr
}

private fun List<Token>.parseUnaryTerm(): ParseNode =
when (head?.isUnaryOperator) {
private fun List<Token>.parseUnaryTerm(): ParseNode {
return when (head?.isUnaryOperator) {
true -> {
val op = head!!

val term = tail.parseUnaryTerm()
var expr: ParseNode? = null
fun makeUnaryParseNode(term: ParseNode) =
ParseNode(UNARY, op, listOf(term), term.remaining)

// constant fold unary plus/minus into constant literals
when (op.keywordText) {
"+" -> when {
term.isNumericLiteral -> {
// unary plus is a NO-OP
expr = term
"+" -> {
val term = tail.parseUnaryTerm()
when {
// unary plus is a no-op on numeric literals.
term.isNumericLiteral -> term
else -> makeUnaryParseNode(term)
}
}
"-" -> when {
term.isNumericLiteral -> {
val num = -term.numberValue()
expr = ParseNode(ATOM,
term.token!!.copy(value = num.ionValue(ion)),
emptyList(),
term.remaining)
"-" -> {
val term = tail.parseUnaryTerm()
when {
// for numbers, drop the minus sign but also negate the value
term.isNumericLiteral ->
term.copy(token = term.token!!.copy(value = (-term.numberValue()).ionValue(ion)))
else -> makeUnaryParseNode(term)
}
}
"not" -> {
val children = tail.parseExpression(op.prefixPrecedence)
expr = ParseNode(UNARY, op, listOf(children), children.remaining)
}
else -> makeUnaryParseNode(tail.parseExpression(op.prefixPrecedence))
}

expr ?: ParseNode(UNARY, op, listOf(term), term.remaining)
}
else -> parsePathTerm()
}
}


private fun List<Token>.parsePathTerm(pathMode: PathMode = PathMode.FULL_PATH): ParseNode {
val term = when (pathMode) {
Expand Down
32 changes: 32 additions & 0 deletions lang/test/org/partiql/lang/syntax/SqlParserTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import org.partiql.lang.ast.SourceLocationMeta
import org.partiql.lang.ast.sourceLocation
import org.partiql.lang.domains.PartiqlAst
import org.partiql.lang.domains.id
import kotlin.concurrent.thread

/**
* Originally just meant to test the parser, this class now tests several different things because
Expand Down Expand Up @@ -3998,4 +3999,35 @@ class SqlParserTest : SqlParserTestBase() {
from = scan(id("bar"))
)))
}

@Test
fun manyNestedNotPerformanceRegressionTest() {
val startTime = System.currentTimeMillis()
val t = thread {
parse(
"""
not not not not not not not not not not not not not not not not not not not not not not not not
not not not not not not not not not not not not not not not not not not not not not not not not
not not not not not not not not not not not not not not not not not not not not not not not not
not not not not not not not not not not not not not not not not not not not not not not not not
not not not not not not not not not not not not not not not not not not not not not not not not
not not not not not not not not not not not not not not not not not not not not not not not not
not not not not not not not not not not not not not not not not not not not not not not not not
not not not not not not not not not not not not not not not not not not not not not not not not
not not not not not not not not not not not not not not not not not not not not not not not not
not not not not not not not not not not not not not not not not not not not not not not not not
not not not not not not not not not not not not not not not not not not not not not not not not
not not not not not not not not not not not not not not not not not not not not not not not not
not not not not not not not not not not not not not not not not not not not not not not not not
not not not not not not not not not not not not not not not not not not not not not not not not false
""")
}
val maxParseTime: Long = 5000
t.join(maxParseTime)
t.interrupt()

assertTrue(
"parsing many nested unary nots should take less than $maxParseTime",
System.currentTimeMillis() - startTime < maxParseTime)
}
}