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

Jump to bottom

SQL: Fix wrong appliance of StackOverflow limit for IN #36724

Merged
merged 4 commits into from
Dec 18, 2018
Merged

SQL: Fix wrong appliance of StackOverflow limit for IN #36724

merged 4 commits into from
Dec 18, 2018

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Dec 17, 2018 •
edited
Loading

Fix grammar so that each element inside the list of values for IN
is a valueExpression and not a more generic expression. Introduce a
mapping for context names as some rules in the grammar are exited with
a different rule from the one they entered.This helps so that the decrement
of depth counts in the Parser's CircuitBreakerListener works correctly.

For the list of values for IN, don't count the
PrimaryExpressionContext as this is not visited on exitRule() due to
the peculiarity in our gramamr with the predicate and predicated.

Fixes: #36592

@matriv matriv requested review from costin and astefan December 17, 2018 16:42
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@matriv matriv added >enhancement and removed >bug labels Dec 17, 2018
@matriv matriv force-pushed the mt/fix-36592 branch 2 times, most recently from 00bb58e to 73a1e1c Compare December 18, 2018 09:26
@matriv
SQL: Fix wrong appliance of StackOverflow limit for IN
3b4053d 
Fix grammar so that each element inside the list of values for IN
is a `valueExpression` and not a more generic `expression`. Also change
some names in the grammar so that they match the primary rule name
plus "Default". This helps so that the decrement of depth counts in
the Parser's `CircuitBreakerListener` works correctly.

For the list of values for `IN` don't count the
`PrimaryExpressionContext` as this is not visited on exit due to
the peculiarity in our gramamr with the `predicate` and `predicated`.

Fixes: elastic#36592
costin
costin approved these changes Dec 18, 2018
View reviewed changes
Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM

astefan
astefan reviewed Dec 18, 2018
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

Left few minor comments.

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/SqlParser.java Outdated

// Skip PrimaryExpressionContext for IN as it's not visited on exit due to
// the grammar's peculiarity rule with "predicated" and "predicate".
// Also skip the Identifiers as they are "cheap".
if (ctx.getClass() != SqlBaseParser.UnquoteIdentifierContext.class &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you import static all these classes to make the code a bit less crowded?

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/SqlParser.java

// Avoid having negative numbers
if (depthCounts.containsKey(classNameToDecrement)) {
depthCounts.putOrAdd(classNameToDecrement, (short) 0, (short) -1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the scenario of negative counter even possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because currently some rules are not encountered on enter but only on exit.
I'll open an issue soon to track this and investigate more.

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/SqlParserTests.java Outdated
@@ -254,6 +261,40 @@ public void testLimitToPreventStackOverflowFromLargeComplexSubselectTree() {
e.getMessage());
}

public void testLimitStackOverflowForInAndLiteralsIsNotApplied() {
new SqlParser().createStatement("SELECT * FROM t WHERE a IN(" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Only the parser being invoked is enough here? Can you check that at least that a minimal plan is created correctly given the query? Also, there is already a method that creates the statement given a query String: parseStatement(String).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test is only to check that we don't hit the limit, but I can add more assertions.

@astefan
Copy link
Contributor

astefan commented Dec 18, 2018

run the gradle build tests 1

astefan
astefan approved these changes Dec 18, 2018
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

@matriv
Copy link
Contributor Author

matriv commented Dec 18, 2018

run the gradle build tests 2

1 similar comment
@matriv
Copy link
Contributor Author

matriv commented Dec 18, 2018

run the gradle build tests 2

@matriv
Copy link
Contributor Author

matriv commented Dec 18, 2018

run the gradle build tests 1

@matriv
Copy link
Contributor Author

matriv commented Dec 18, 2018

run the gradle build tests 2

@matriv matriv merged commit 74502d7 into elastic:master Dec 18, 2018
@matriv matriv deleted the mt/fix-36592 branch December 18, 2018 15:07
matriv added a commit that referenced this pull request Dec 18, 2018
@matriv
SQL: Fix wrong appliance of StackOverflow limit for IN (#36724)
7d2e2cf 
Fix grammar so that each element inside the list of values for IN
is a valueExpression and not a more generic expression. Introduce a
mapping for context names as some rules in the grammar are exited with
a different rule from the one they entered.This helps so that the decrement
of depth counts in the Parser's CircuitBreakerListener works correctly.

For the list of values for IN, don't count the
PrimaryExpressionContext as this is not visited on exitRule() due to
the peculiarity in our gramamr with the predicate and predicated.

Fixes: #36592
@matriv
Copy link
Contributor Author

matriv commented Dec 18, 2018

Backported to 6.x with 7d2e2cf

matriv added a commit that referenced this pull request Dec 18, 2018
@matriv
SQL: Fix wrong appliance of StackOverflow limit for IN (#36724)
7fa3154 
Fix grammar so that each element inside the list of values for IN
is a valueExpression and not a more generic expression. Introduce a
mapping for context names as some rules in the grammar are exited with
a different rule from the one they entered.This helps so that the decrement
of depth counts in the Parser's CircuitBreakerListener works correctly.

For the list of values for IN, don't count the
PrimaryExpressionContext as this is not visited on exitRule() due to
the peculiarity in our gramamr with the predicate and predicated.

Fixes: #36592
@matriv
Copy link
Contributor Author

matriv commented Dec 18, 2018

Backported to 6.5 with 7fa3154

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Dec 18, 2018
@jasontedor
Merge remote-tracking branch 'elastic/master' into lazier-ingest-geoip
a674600 
* elastic/master: (31 commits)
  enable bwc tests and switch transport serialization version to 6.6.0 for CAS features
  [DOCs] Adds ml-cpp PRs to alpha release notes (elastic#36790)
  Synchronize WriteReplicaResult callbacks (elastic#36770)
  Add CcrRestoreSourceService to track sessions (elastic#36578)
  [Painless] Add tests for boxed return types (elastic#36747)
  Internal: Remove originalSettings from Node (elastic#36569)
  [ILM][DOCS] Update ILM API authorization docs (elastic#36749)
  Core: Deprecate use of scientific notation in epoch time parsing (elastic#36691)
  [ML] Merge the Jindex master feature branch (elastic#36702)
  Tests: Mute SnapshotDisruptionIT.testDisruptionOnSnapshotInitialization
  Update versions in SearchSortValues transport serialization
  Update version in SearchHits transport serialization
  [Geo] Integrate Lucene's LatLonShape (BKD Backed GeoShapes) as default `geo_shape` indexing approach (elastic#36751)
  [Docs] Fix error in Common Grams Token Filter (elastic#36774)
  Fix rollup search statistics (elastic#36674)
  SQL: Fix wrong appliance of StackOverflow limit for IN (elastic#36724)
  [TEST] Added more logging
  Invalidate Token API enhancements - HLRC (elastic#36362)
  Deprecate types in index API (elastic#36575)
  Disable bwc tests until elastic#36555 backport is complete (elastic#36737)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astefan astefan astefan approved these changes

@costin costin costin approved these changes

Assignees
No one assigned
Labels
:Analytics/SQL SQL querying >enhancement v6.5.5 v6.6.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@matriv @elasticmachine @astefan @costin @colings86