Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21729 )
Change subject: IMPALA-13185: Include runtime filter source in key ...................................................................... Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/21729/5/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: http://gerrit.cloudera.org:8080/#/c/21729/5/fe/src/main/java/org/apache/impala/planner/PlanNode.java@544 PS5, Line 544: msg.addToRuntime_filters(filter.toThrift(serialCtx)); The other option is to pass in this PlanNode to this toThrift() call separately from the serialization context and use that to filter the targets. I think we only need it for the duration of this call. http://gerrit.cloudera.org:8080/#/c/21729/5/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1362 PS5, Line 1362: // Build may not have been visited yet. Here are some ways that I test by hand (with Impala running with tuple caching set up): # It's nice to have a table that we can insert into create table testtbl(i int); insert into testtbl values (1); # Below is a query that has multiple runtime filters and three caching locations. # What I see today: # - The probe side has references to multiple "input scan node ids" (i.e. 2,2,1,0) # - When running with num_nodes=1, this works and inserting a row into testtbl would cause all the keys to change. # - When running with num_nodes=0 (with or without mt_dop), this won't notice a new insert into testtbl. # # How it really should be: # - The probe side only has "input scan node ids" that are its children (i.e. it gets none from runtime filters) # - The keys are different when something inserts into testtbl. select straight_join a.id from functional_parquet.alltypes a, functional_parquet.alltypes b, testbl c where a.id = b.id and a.id = c.i; http://gerrit.cloudera.org:8080/#/c/21729/5/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1364 PS5, Line 1364: if (!tupleCacheInfo_.mergeChild(build.getTupleCacheInfo())) { This merge is going to be special. We will want to squash the scan ranges at this point (which will also null out the list of input scan node ids). Right now, this works for single-node plans because they are in a single fragment. At run time, when we go to hash the scan ranges, the fragment can see all of them from the various scan nodes including build side scan nodes that matter due to runtime filters. However, that only works for single node plans. Once we get to multiple fragments, it can't get the right information at run time. http://gerrit.cloudera.org:8080/#/c/21729/5/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/21729/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@395 PS5, Line 395: tFilter.setNdv_estimate(ndvEstimate_); Nit: We might want to skip this for tuple caching. http://gerrit.cloudera.org:8080/#/c/21729/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@403 PS5, Line 403: if (serialCtx.includeRFTarget(target.node)) { : // Only serialize the relevant target PlanNode when using tuple caching. Nit: Can we add a bit more to the comment say that we're skipping runtime filters that don't target this plan node? http://gerrit.cloudera.org:8080/#/c/21729/5/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java File fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java: http://gerrit.cloudera.org:8080/#/c/21729/5/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java@69 PS5, Line 69: public void testRuntimeFilterCacheKeys() { Something that can help with tests that use joins is to use straight_join to force a particular join order. We can use that to make it clear which is the build side producing the filter. As an example, here is a query that has a join on its build side and would not have caching on the probe side: select straight_join probe.id from functional.alltypes probe, (select build1.id from functional.alltypes build1, functional.alltypestiny build2 where build1.id = build2.id) build where probe.id = build.id; http://gerrit.cloudera.org:8080/#/c/21729/5/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java@81 PS5, Line 81: verifyDifferentCacheKeys(basicJoin, "select a.id from functional.alltypes a, " + : "functional.alltypes b where a.id = b.id"); In addition to this test case, let's add one where the build side has an additional filter. http://gerrit.cloudera.org:8080/#/c/21729/5/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java@111 PS5, Line 111: public void testIneligibility() { >From an eligibility point of view, we can add some tests where a Kudu table >produces a runtime filter and that causes a scan of an HDFS table to be >ineligible for caching. http://gerrit.cloudera.org:8080/#/c/21729/5/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java@139 PS5, Line 139: public void testCacheKeyGenerality() { There's a way we can add a test for this. Basically, that query that has a join on its build side: select straight_join probe.id from functional.alltypes probe, (select build1.id from functional.alltypes build1, functional.alltypestiny build2 where build1.id = build2.id) build where probe.id = build.id; This query has two tuple cache locations for each of the build-side scans that feed into the join on the build side. If we add a bunch of additional filters on the probe side table, that changes the slot ids. I think we should get identical keys for those two build-side caching locations. http://gerrit.cloudera.org:8080/#/c/21729/5/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java@371 PS5, Line 371: queryOptions.setNum_nodes(1); One quirk of this test so far is that it uses num_nodes=1, so there are no exchanges. To test looking past an exchange, we might want some test cases where num_nodes!=1 and the plan has exchanges. -- To view, visit http://gerrit.cloudera.org:8080/21729 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0077964be5acdb588d76251a6a39e57a0f42bb5a Gerrit-Change-Number: 21729 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Thu, 12 Sep 2024 23:50:35 +0000 Gerrit-HasComments: Yes
