Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14012 )
Change subject: IMPALA-4551: Limit the size of SQL statements ...................................................................... Patch Set 1: (4 comments) lgtm overall. Some general comments. 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: analysisResult_.analyzer_.checkStmtExprLimit(); nit: Probably worth mentioning that we want to enforce this before the rewrites because rewrites are costly with long expression chains. 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? 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 looks to me like they are accounted, add tests for a mix of Literal and Const expressions? 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 set a non-default lower expression limit to save time and memory. -- 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: 1 Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Fri, 09 Aug 2019 17:11:34 +0000 Gerrit-HasComments: Yes
