Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/14012 )
Change subject: IMPALA-4551: Limit the size of SQL statements ...................................................................... Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/14012/2/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/14012/2/be/src/service/query-options.h@190 PS2, Line 190: > nit: Do we need spaces here according our code style? Removed the spaces for these new lines. I don't think the spaces are needed for any code style thing. http://gerrit.cloudera.org:8080/#/c/14012/2/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/14012/2/be/src/service/query-options.cc@824 PS2, Line 824: Invalid statement expression limit > nit: can we also print the value? Added the actual value here and for the max statement length bytes. http://gerrit.cloudera.org:8080/#/c/14012/2/be/src/service/query-options.cc@825 PS2, Line 825: Value > typo: Valid Done http://gerrit.cloudera.org:8080/#/c/14012/2/be/src/service/query-options.cc@838 PS2, Line 838: value > nit: values Done http://gerrit.cloudera.org:8080/#/c/14012/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/14012/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@462 PS1, Line 462: boolean isExplain = analysisResult_.isExplainStmt(); > nit: Probably worth mentioning that we want to enforce this before the rewr Done http://gerrit.cloudera.org:8080/#/c/14012/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/14012/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2861 PS2, Line 2861: String errorStr = String.format("Exceeded the statement expression limit (%s)\n" + > nit: use "%d" for int. Switched to %d. I don't know the history, but it makes sense to use %d. http://gerrit.cloudera.org:8080/#/c/14012/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/14012/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2435 PS1, Line 2435: String repCols20 = getRepeatedColumnReference("int_col", 20, true); > Verify that analyzer.numStmtExprs_ is accounted properly? Added asserts for the AnalyzesOk() calls to verify numStmtExprs_ is exactly what we expect. The AnalysisError() cases already include the numStmtExprs_ in the error message. http://gerrit.cloudera.org:8080/#/c/14012/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2493 PS1, Line 2493: StringBuilder inList = new StringBuilder(); > what about constant expressions in the IN lists? foo IN (2*3, 3*4...) ? It Good point, I added tests for arithmetic expressions 1*2*3*4. This found a bug where these expressions could be double-counted due to the Exprs being cloned and reanalyzed. I added a per-Expr variable to track whether the Expr had been counted. Each Expr will only be counted once. This also required some logic to skip counting these expressions when in a WITH clause (it gets accounted separately). I added a test that has an IN list with an arithmetic expression. http://gerrit.cloudera.org:8080/#/c/14012/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/14012/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2443 PS2, Line 2443: // WHERE clause > nit: For coverage, maybe test the expression in the WHERE clause? The way I'm thinking about this is that the SQL without the WHERE has 20 expressions and adding the WHERE with bool_col adds one expression, so it would only fail if the WHERE were counted. I added a comment here. http://gerrit.cloudera.org:8080/#/c/14012/1/tests/query_test/test_exprs.py File tests/query_test/test_exprs.py: http://gerrit.cloudera.org:8080/#/c/14012/1/tests/query_test/test_exprs.py@139 PS1, Line 139: # This takes 20+ minutes, so only run it on exhaustive. > Is the intention to test the default limits here? If not, we can probably s I wanted this to test as close to the default limits as possible. It is unclear how much value this test has. In my runs, it doesn't seem to impact the total runtime of the exhaustive tests by much. http://gerrit.cloudera.org:8080/#/c/14012/2/tests/query_test/test_exprs.py File tests/query_test/test_exprs.py: http://gerrit.cloudera.org:8080/#/c/14012/2/tests/query_test/test_exprs.py@216 PS2, Line 216: > flake8: E261 at least two spaces before inline comment Done -- To view, visit http://gerrit.cloudera.org:8080/14012 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5675fb4a08c1dc51ae5bcf467cbb969cc064602c Gerrit-Change-Number: 14012 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 13 Aug 2019 01:07:32 +0000 Gerrit-HasComments: Yes
