impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3548: Prune runtime filters based on query options in the FE
Date Tue, 24 Oct 2017 23:47:32 GMT
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/7564 )

Change subject: IMPALA-3548: Prune runtime filters based on query options in the FE
......................................................................


Patch Set 11:

(13 comments)

Looks good, final comments.

http://gerrit.cloudera.org:8080/#/c/7564/10/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

http://gerrit.cloudera.org:8080/#/c/7564/10/be/src/service/fe-support.cc@455
PS10, Line 455:   TQueryOptions options;
> I've renamed the parameter.
Thanks for investigating and providing an explanation!

We should summarize your findings and add them to the function comment of FeSupport#NativeParseQueryOptions()
to explain why we must pass in an existing TQueryOptions for this to work as intended.


http://gerrit.cloudera.org:8080/#/c/7564/11/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

http://gerrit.cloudera.org:8080/#/c/7564/11/be/src/service/fe-support.cc@453
PS11, Line 453: Java_org_apache_impala_service_FeSupport_NativeParseQueryOptions(
Nice and clean! Thanks.


http://gerrit.cloudera.org:8080/#/c/7564/11/be/src/service/fe-support.cc@454
PS11, Line 454:     JNIEnv* env, jclass caller_class, jstring options_str, jbyteArray tquery_options)
{
rename options_str to csv_query_options for consistency


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java:

http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@397
PS10, Line 397:     int maxNumFilters = ctx.getQueryOptions().getMax_num_runtime_filters();
> 0 is a valid value. MAX_NUM_RUNTIME_FILTERS=0 effectively removes every run
Sorry, my mistake. For some reason I didn't read the ">=" correctly.


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/service/FeSupport.java
File fe/src/main/java/org/apache/impala/service/FeSupport.java:

http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/service/FeSupport.java@93
PS10, Line 93: 
> See my response to be/src/service/fe-support.cc L455.
Yikes! What a can of worms. Thanks for investigating and explaining.


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/test/java/org/apache/impala/testutil/TestFileParser.java
File fe/src/test/java/org/apache/impala/testutil/TestFileParser.java:

http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/test/java/org/apache/impala/testutil/TestFileParser.java@288
PS10, Line 288:         if (!currentTestCase.isValid()) {
> L288-L293 handles test cases found in the middle of the test file.
All PlannerTest .test files should end with "====", so I think the code here in L228 is enough.
I tried your patch locally using a .test file with 3 tests and the query option parsing worked
as expected without L338.

I might be missing something, if so, can you point me to an example file where we also need
the parseQueryOptions() call in L338?


http://gerrit.cloudera.org:8080/#/c/7564/11/fe/src/test/java/org/apache/impala/testutil/TestFileParser.java
File fe/src/test/java/org/apache/impala/testutil/TestFileParser.java:

http://gerrit.cloudera.org:8080/#/c/7564/11/fe/src/test/java/org/apache/impala/testutil/TestFileParser.java@359
PS11, Line 359:           " - " + e.getMessage());
also pass in 'e' as the cause


http://gerrit.cloudera.org:8080/#/c/7564/11/testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-query-options.test
File testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-query-options.test:

http://gerrit.cloudera.org:8080/#/c/7564/11/testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-query-options.test@38
PS11, Line 38: ---- DISTRIBUTEDPLAN
What's the value in keeping this section?


http://gerrit.cloudera.org:8080/#/c/7564/11/testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-query-options.test@174
PS11, Line 174: ---- DISTRIBUTEDPLAN
What's the value in keeping this section?


http://gerrit.cloudera.org:8080/#/c/7564/11/testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-query-options.test@257
PS11, Line 257: ---- DISTRIBUTEDPLAN
What's the value in keeping this section?


http://gerrit.cloudera.org:8080/#/c/7564/11/testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-query-options.test@392
PS11, Line 392: MAX_NUM_RUNTIME_FILTERS=4
How about setting to 3 so we can see both local and non-local filters being removed from the
distributed plan


http://gerrit.cloudera.org:8080/#/c/7564/11/testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-query-options.test@582
PS11, Line 582: ---- DISTRIBUTEDPLAN
What's the value in keeping this section?


http://gerrit.cloudera.org:8080/#/c/7564/11/testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-query-options.test@657
PS11, Line 657: ---- DISTRIBUTEDPLAN
What's the value in keeping this section?



-- 
To view, visit http://gerrit.cloudera.org:8080/7564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id0f0b200e02442edcad8df3979f652d66c6e52eb
Gerrit-Change-Number: 7564
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mjacobs@apache.org>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Oct 2017 23:47:32 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message