You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2024/06/20 04:52:34 UTC

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21541

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................

IMPALA-12906: Incorporate scan range information into the tuple cache key

This change is accomplishing two things:
1. It incorporates scan range information into the tuple
cache key.
2. It reintroduces deterministic scheduling as an option
for mt_dop and uses it for HdfsScanNodes that feed
into a TupleCacheNode.

The combination of these two things solves several problems:
1. When a table is modified, the list of scan ranges will
change, and this will naturally change the cache keys.
2. When accessing a partitioned table, two queries may have
different predicates on the partition columns. Since the
predicates can be satisfied via partition pruning, they are
not included at runtime. This means that two queries
may have identical compile-time keys with only the scan
ranges being different due to different partition pruning.
3. Each fragment instance processes different scan ranges, so
each will have a unique cache key.

To incorporate scan range information, this introduces a new
per-fragment-instance cache key. At compile time, the planner
now keeps track of which HdfsScanNodes feed into a TupleCacheNode.
This is passed over to the runtime as a list of plan node ids
that contain scan ranges. At runtime, the fragment instance
walks through the list of plan nodes ids and hashes any scan ranges
associated with them. This hash is the per-fragment-instance
cache key. The combination of the compile-time cache key produced
by the planner and the per-fragment-instance cache key is a unique
identifier of the result.

Deterministic scheduling for mt_dop was removed via IMPALA-9655
with the introduction of the shared queue. This revives some of
the pre-IMPALA-9655 scheduling logic as an option. Since the
TupleCacheNode knows which HdfsScanNodes feed into it, the
TupleCacheNode turns on deterministic scheduling for all of those
HdfsScanNodes. Since this only applies to HdfsScanNodes that feed
into a TupleCacheNode, it means that any HdfsScanNode that doesn't
feed into a TupleCacheNode continues using the current algorithm.

Testing:
- Added custom cluster tests for the scan range information
including modifying a table, selecting from a partitioned
table, and verifying that fragment instances have unique
keys
- Added basic frontend test to verify that deterministic scheduling
gets set on the HdfsScanNode that feed into the TupleCacheNode.

Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/tuple-cache-node.cc
M be/src/exec/tuple-cache-node.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/common/ThriftSerializationCtx.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
15 files changed, 514 insertions(+), 50 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/21541/1
--
To view, visit http://gerrit.cloudera.org:8080/21541
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Joe McDonnell has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................

IMPALA-12906: Incorporate scan range information into the tuple cache key

Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Reviewed-on: http://gerrit.cloudera.org:8080/21541
Reviewed-by: Michael Smith <mi...@cloudera.com>
Tested-by: Michael Smith <mi...@cloudera.com>
Reviewed-by: Yida Wu <wy...@gmail.com>
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/tuple-cache-node.cc
M be/src/exec/tuple-cache-node.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/common/ThriftSerializationCtx.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
15 files changed, 879 insertions(+), 69 deletions(-)

Approvals:
Michael Smith: Looks good to me, but someone else must approve; Verified
Yida Wu: Looks good to me, approved

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 12
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has uploaded a new patch set (#11) to the change originally created by Joe McDonnell. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................

IMPALA-12906: Incorporate scan range information into the tuple cache key

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/21541/11
--
To view, visit http://gerrit.cloudera.org:8080/21541
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 11
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 4: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 31 Jul 2024 21:40:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16736/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Aug 2024 21:15:35 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 8:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10906/ DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 8
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 27 Aug 2024 16:35:21 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21541/2/tests/custom_cluster/test_tuple_cache.py
File tests/custom_cluster/test_tuple_cache.py:

http://gerrit.cloudera.org:8080/#/c/21541/2/tests/custom_cluster/test_tuple_cache.py@88
PS2, Line 88:   # Get cache metrics from all impalads
This comment seems wrong, it's just getting it from a single impalad.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 28 Jun 2024 23:51:25 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16735/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Aug 2024 20:03:50 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.

Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 11: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 11
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 04 Sep 2024 16:29:14 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21541/7/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

http://gerrit.cloudera.org:8080/#/c/21541/7/be/src/scheduling/scheduler.cc@593
PS7, Line 593:   if (a_weight != b_weight) {
> nit: Could be a one-liner. As could the assignment above.
Done


http://gerrit.cloudera.org:8080/#/c/21541/7/be/src/scheduling/scheduler.cc@603
PS7, Line 603:     if (a_hdfs_split.mtime() != b_hdfs_split.mtime()) {
> If these get more expensive to compute, do we really want to repeat them fo
In terms of generated code, for const accessor methods on const references, the compiler will optimize it properly. Using local variables won't change the generated code here.

If we do more expensive things, then we should be careful and use local variables. Something like ScanRangeWeight() should get optimized properly, but I'm being cautious there.

If there is a better style, I'm open to it. I didn't find local variables to help much.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 26 Aug 2024 22:48:07 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 11: Verified+1 Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 11
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 27 Aug 2024 22:59:43 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 2:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/21541/1/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:

http://gerrit.cloudera.org:8080/#/c/21541/1/be/src/exec/hdfs-scan-node-base.cc@171
PS1, Line 171:   const string& avro_schema_str = hdfs_table_->avro_schema();
> Good point, dropped this extra field
Ack


http://gerrit.cloudera.org:8080/#/c/21541/1/be/src/exec/tuple-cache-node.h
File be/src/exec/tuple-cache-node.h:

http://gerrit.cloudera.org:8080/#/c/21541/1/be/src/exec/tuple-cache-node.h@62
PS1, Line 62:   // This combination is unique for a given fragment instance.
> Changed this to drop input_scan_node_ids_ and compile_time_key_ as fields o
I think we have a pattern of making unnecessary copy of Thrift fields. May be worth a pass at some point.


http://gerrit.cloudera.org:8080/#/c/21541/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

http://gerrit.cloudera.org:8080/#/c/21541/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1959
PS1, Line 1959:     // There reason we skip setting the deterministic scan range assignment field for
> I added a comment here. Basically, for computing the tuple cache key, this 
Done


http://gerrit.cloudera.org:8080/#/c/21541/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

http://gerrit.cloudera.org:8080/#/c/21541/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1959
PS2, Line 1959:     // There reason we skip setting the deterministic scan range assignment field for
nit: "The" not "There"


http://gerrit.cloudera.org:8080/#/c/21541/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@2065
PS2, Line 2065:       // To avoid polluting the explain output, only add this if this is mt_dop>0 and
nit: "this" is used in two different ways here. I'd shorten it to "only add this if mt_dop>0 and..."


http://gerrit.cloudera.org:8080/#/c/21541/1/tests/custom_cluster/test_tuple_cache.py
File tests/custom_cluster/test_tuple_cache.py:

http://gerrit.cloudera.org:8080/#/c/21541/1/tests/custom_cluster/test_tuple_cache.py@196
PS1, Line 196:       result = self.cached_query_w_debugaction(query,
> Added a base class for these tests and pulled these tests out into their ow
Ack



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 28 Jun 2024 23:46:42 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 9:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16818/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 9
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 27 Aug 2024 23:24:42 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 11:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16820/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 11
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 27 Aug 2024 23:38:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Hello Kurt Deschler, Yida Wu, Michael Smith, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21541

to look at the new patch set (#7).

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................

IMPALA-12906: Incorporate scan range information into the tuple cache key

This change is accomplishing two things:
1. It incorporates scan range information into the tuple
   cache key.
2. It reintroduces deterministic scheduling as an option
   for mt_dop and uses it for HdfsScanNodes that feed
   into a TupleCacheNode.

The combination of these two things solves several problems:
1. When a table is modified, the list of scan ranges will
   change, and this will naturally change the cache keys.
2. When accessing a partitioned table, two queries may have
   different predicates on the partition columns. Since the
   predicates can be satisfied via partition pruning, they are
   not included at runtime. This means that two queries
   may have identical compile-time keys with only the scan
   ranges being different due to different partition pruning.
3. Each fragment instance processes different scan ranges, so
   each will have a unique cache key.

To incorporate scan range information, this introduces a new
per-fragment-instance cache key. At compile time, the planner
now keeps track of which HdfsScanNodes feed into a TupleCacheNode.
This is passed over to the runtime as a list of plan node ids
that contain scan ranges. At runtime, the fragment instance
walks through the list of plan nodes ids and hashes any scan ranges
associated with them. This hash is the per-fragment-instance
cache key. The combination of the compile-time cache key produced
by the planner and the per-fragment-instance cache key is a unique
identifier of the result.

Deterministic scheduling for mt_dop was removed via IMPALA-9655
with the introduction of the shared queue. This revives some of
the pre-IMPALA-9655 scheduling logic as an option. Since the
TupleCacheNode knows which HdfsScanNodes feed into it, the
TupleCacheNode turns on deterministic scheduling for all of those
HdfsScanNodes. Since this only applies to HdfsScanNodes that feed
into a TupleCacheNode, it means that any HdfsScanNode that doesn't
feed into a TupleCacheNode continues using the current algorithm.
The pre-IMPALA-9655 code is modified to make it more deterministic
about how it assigns scan ranges to instances.

Testing:
 - Added custom cluster tests for the scan range information
   including modifying a table, selecting from a partitioned
   table, and verifying that fragment instances have unique
   keys
 - Added basic frontend test to verify that deterministic scheduling
   gets set on the HdfsScanNode that feed into the TupleCacheNode.
 - Restored the pre-IMPALA-9655 backend test to cover the LPT code

Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/tuple-cache-node.cc
M be/src/exec/tuple-cache-node.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/common/ThriftSerializationCtx.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
15 files changed, 753 insertions(+), 69 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/21541/7
-- 
To view, visit http://gerrit.cloudera.org:8080/21541
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21541/7/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

http://gerrit.cloudera.org:8080/#/c/21541/7/be/src/scheduling/scheduler.cc@603
PS7, Line 603:     const HdfsFileSplitPB& a_hdfs_split = a.scan_range().hdfs_file_split();
> Yeah, the definitions of the accessor functions are in the header file, and
Ack



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 8
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 27 Aug 2024 18:24:37 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 8: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21541/7/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

http://gerrit.cloudera.org:8080/#/c/21541/7/be/src/scheduling/scheduler.cc@593
PS7, Line 593:   if (a_weight != b_weight) return a_weight > b_weight;
> Done
Ack


http://gerrit.cloudera.org:8080/#/c/21541/7/be/src/scheduling/scheduler.cc@603
PS7, Line 603:     const HdfsFileSplitPB& a_hdfs_split = a.scan_range().hdfs_file_split();
> In terms of generated code, for const accessor methods on const references,
My point was related to the comment at lines 601-602. This is fine unless we see actually measurable perf problems. I'm pleasantly surprised if the compiler optimizes these to a single function call each.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 8
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 26 Aug 2024 22:55:06 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 1: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/10743/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Jun 2024 22:08:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/21541/1/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:

http://gerrit.cloudera.org:8080/#/c/21541/1/be/src/exec/hdfs-scan-node-base.cc@171
PS1, Line 171:     deterministic_scanrange_assignment_ =
> It's a little silly we copy all these values when tnode is preserved in Pla
Good point, dropped this extra field


http://gerrit.cloudera.org:8080/#/c/21541/1/be/src/exec/tuple-cache-node.h
File be/src/exec/tuple-cache-node.h:

http://gerrit.cloudera.org:8080/#/c/21541/1/be/src/exec/tuple-cache-node.h@62
PS1, Line 62:   const std::vector<int32_t> input_scan_node_ids_;
> nit: Could these be references to the tnode_ data? Or does that have a diff
Changed this to drop input_scan_node_ids_ and compile_time_key_ as fields on this class. The code can just refer to the field on the plan node directly.


http://gerrit.cloudera.org:8080/#/c/21541/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

http://gerrit.cloudera.org:8080/#/c/21541/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1959
PS1, Line 1959:     if (!serialCtx.isTupleCache()) {
> I don't understand this conditional.
I added a comment here. Basically, for computing the tuple cache key, this doesn't add any information.


http://gerrit.cloudera.org:8080/#/c/21541/1/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
File fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java:

http://gerrit.cloudera.org:8080/#/c/21541/1/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java@125
PS1, Line 125:     return output.toString();
> Should we print input scan nodes here?
Good idea, added that here


http://gerrit.cloudera.org:8080/#/c/21541/1/tests/custom_cluster/test_tuple_cache.py
File tests/custom_cluster/test_tuple_cache.py:

http://gerrit.cloudera.org:8080/#/c/21541/1/tests/custom_cluster/test_tuple_cache.py@196
PS1, Line 196:     for mt_dop in [0, 1]:
> Could this be done with @pytest.mark.parametrize instead?
Added a base class for these tests and pulled these tests out into their own class. This now has mt_dop as a test dimension.


http://gerrit.cloudera.org:8080/#/c/21541/1/tests/custom_cluster/test_tuple_cache.py@287
PS1, Line 287:   def test_scan_range_distributed(self, vector, unique_database):
> All these tests appear to rely entirely on the runtime profile. Can we also
I added checks to make sure the results overlap properly for each of these tests. 

I added some checks for the profile metrics for the single-node tests.

I also added checks for the number of entries in the cache at a daemon level for the distributed test.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 28 Jun 2024 23:39:50 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Hello Kurt Deschler, Yida Wu, Michael Smith, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21541

to look at the new patch set (#5).

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................

IMPALA-12906: Incorporate scan range information into the tuple cache key

This change is accomplishing two things:
1. It incorporates scan range information into the tuple
   cache key.
2. It reintroduces deterministic scheduling as an option
   for mt_dop and uses it for HdfsScanNodes that feed
   into a TupleCacheNode.

The combination of these two things solves several problems:
1. When a table is modified, the list of scan ranges will
   change, and this will naturally change the cache keys.
2. When accessing a partitioned table, two queries may have
   different predicates on the partition columns. Since the
   predicates can be satisfied via partition pruning, they are
   not included at runtime. This means that two queries
   may have identical compile-time keys with only the scan
   ranges being different due to different partition pruning.
3. Each fragment instance processes different scan ranges, so
   each will have a unique cache key.

To incorporate scan range information, this introduces a new
per-fragment-instance cache key. At compile time, the planner
now keeps track of which HdfsScanNodes feed into a TupleCacheNode.
This is passed over to the runtime as a list of plan node ids
that contain scan ranges. At runtime, the fragment instance
walks through the list of plan nodes ids and hashes any scan ranges
associated with them. This hash is the per-fragment-instance
cache key. The combination of the compile-time cache key produced
by the planner and the per-fragment-instance cache key is a unique
identifier of the result.

Deterministic scheduling for mt_dop was removed via IMPALA-9655
with the introduction of the shared queue. This revives some of
the pre-IMPALA-9655 scheduling logic as an option. Since the
TupleCacheNode knows which HdfsScanNodes feed into it, the
TupleCacheNode turns on deterministic scheduling for all of those
HdfsScanNodes. Since this only applies to HdfsScanNodes that feed
into a TupleCacheNode, it means that any HdfsScanNode that doesn't
feed into a TupleCacheNode continues using the current algorithm.
The pre-IMPALA-9655 code is modified to make it more deterministic
about how it assigns scan ranges to instances.

Testing:
 - Added custom cluster tests for the scan range information
   including modifying a table, selecting from a partitioned
   table, and verifying that fragment instances have unique
   keys
 - Added basic frontend test to verify that deterministic scheduling
   gets set on the HdfsScanNode that feed into the TupleCacheNode.
 - Restored the pre-IMPALA-9655 backend test to cover the LPT code

Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/tuple-cache-node.cc
M be/src/exec/tuple-cache-node.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/common/ThriftSerializationCtx.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
15 files changed, 739 insertions(+), 69 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/21541/5
-- 
To view, visit http://gerrit.cloudera.org:8080/21541
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Hello Kurt Deschler, Yida Wu, Michael Smith, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21541

to look at the new patch set (#8).

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................

IMPALA-12906: Incorporate scan range information into the tuple cache key

This change is accomplishing two things:
1. It incorporates scan range information into the tuple
   cache key.
2. It reintroduces deterministic scheduling as an option
   for mt_dop and uses it for HdfsScanNodes that feed
   into a TupleCacheNode.

The combination of these two things solves several problems:
1. When a table is modified, the list of scan ranges will
   change, and this will naturally change the cache keys.
2. When accessing a partitioned table, two queries may have
   different predicates on the partition columns. Since the
   predicates can be satisfied via partition pruning, they are
   not included at runtime. This means that two queries
   may have identical compile-time keys with only the scan
   ranges being different due to different partition pruning.
3. Each fragment instance processes different scan ranges, so
   each will have a unique cache key.

To incorporate scan range information, this introduces a new
per-fragment-instance cache key. At compile time, the planner
now keeps track of which HdfsScanNodes feed into a TupleCacheNode.
This is passed over to the runtime as a list of plan node ids
that contain scan ranges. At runtime, the fragment instance
walks through the list of plan nodes ids and hashes any scan ranges
associated with them. This hash is the per-fragment-instance
cache key. The combination of the compile-time cache key produced
by the planner and the per-fragment-instance cache key is a unique
identifier of the result.

Deterministic scheduling for mt_dop was removed via IMPALA-9655
with the introduction of the shared queue. This revives some of
the pre-IMPALA-9655 scheduling logic as an option. Since the
TupleCacheNode knows which HdfsScanNodes feed into it, the
TupleCacheNode turns on deterministic scheduling for all of those
HdfsScanNodes. Since this only applies to HdfsScanNodes that feed
into a TupleCacheNode, it means that any HdfsScanNode that doesn't
feed into a TupleCacheNode continues using the current algorithm.
The pre-IMPALA-9655 code is modified to make it more deterministic
about how it assigns scan ranges to instances.

Testing:
 - Added custom cluster tests for the scan range information
   including modifying a table, selecting from a partitioned
   table, and verifying that fragment instances have unique
   keys
 - Added basic frontend test to verify that deterministic scheduling
   gets set on the HdfsScanNode that feed into the TupleCacheNode.
 - Restored the pre-IMPALA-9655 backend test to cover the LPT code

Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/tuple-cache-node.cc
M be/src/exec/tuple-cache-node.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/common/ThriftSerializationCtx.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
15 files changed, 879 insertions(+), 69 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/21541/8
-- 
To view, visit http://gerrit.cloudera.org:8080/21541
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 8
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 8: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 8
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 27 Aug 2024 21:34:17 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16596/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 31 Jul 2024 17:54:35 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 1:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10743/ DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Jun 2024 17:04:38 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16447/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 29 Jun 2024 00:27:53 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/21541/1/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:

http://gerrit.cloudera.org:8080/#/c/21541/1/be/src/exec/hdfs-scan-node-base.cc@171
PS1, Line 171:     deterministic_scanrange_assignment_ =
It's a little silly we copy all these values when tnode is preserved in PlanNode.


http://gerrit.cloudera.org:8080/#/c/21541/1/be/src/exec/tuple-cache-node.h
File be/src/exec/tuple-cache-node.h:

http://gerrit.cloudera.org:8080/#/c/21541/1/be/src/exec/tuple-cache-node.h@62
PS1, Line 62:   const std::vector<int32_t> input_scan_node_ids_;
nit: Could these be references to the tnode_ data? Or does that have a different lifetime?


http://gerrit.cloudera.org:8080/#/c/21541/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

http://gerrit.cloudera.org:8080/#/c/21541/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1959
PS1, Line 1959:     if (!serialCtx.isTupleCache()) {
I don't understand this conditional.


http://gerrit.cloudera.org:8080/#/c/21541/1/tests/custom_cluster/test_tuple_cache.py
File tests/custom_cluster/test_tuple_cache.py:

http://gerrit.cloudera.org:8080/#/c/21541/1/tests/custom_cluster/test_tuple_cache.py@196
PS1, Line 196:     for mt_dop in [0, 1]:
Could this be done with @pytest.mark.parametrize instead?


http://gerrit.cloudera.org:8080/#/c/21541/1/tests/custom_cluster/test_tuple_cache.py@287
PS1, Line 287:   def test_scan_range_distributed(self, vector, unique_database):
All these tests appear to rely entirely on the runtime profile. Can we also assert that different cache entries were created via a different method, like different results and updated metrics?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Jun 2024 18:16:10 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16446/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 29 Jun 2024 00:06:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10770/ DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 29 Jun 2024 00:28:52 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.

Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 12: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 12
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 04 Sep 2024 21:45:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21541/6/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

http://gerrit.cloudera.org:8080/#/c/21541/6/be/src/scheduling/scheduler.cc@601
PS6, Line 601:     if (a_hdfs_split.mtime() > b_hdfs_split.mtime()) return true;
I think this is insufficient. If a.mtime < b.mtime and a.offset > b.offset, then ScanRangeComparator(a, b) is true and ScanRangeComparator(b, a) is true. That doesn't satisfy Compare. So I think - given a predicate function Pred - you need to do the full

  if (Pred(a) > Pred(b)) return true;
  if (Pred(a) < Pred(b)) return false;

Could simplify to

  if (auto pa = Pred(a), pb = Pred(b); pa != pb) return pa > pb;

I'd recommend using tuple comparison, but I don't think there's a lazy version.


http://gerrit.cloudera.org:8080/#/c/21541/6/be/src/scheduling/scheduler.cc@637
PS6, Line 637:       // To make this determinstic, break ties by comparing the instance idxs
nit: deterministic



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 6
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Aug 2024 20:14:32 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21541/7/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

http://gerrit.cloudera.org:8080/#/c/21541/7/be/src/scheduling/scheduler.cc@603
PS7, Line 603:     if (a_hdfs_split.mtime() != b_hdfs_split.mtime()) {
> My point was related to the comment at lines 601-602. This is fine unless w
Yeah, the definitions of the accessor functions are in the header file, and the compiler knows everything. Trying out similar code on Godbolt, the code ends up being two memory accesses, one cmp, and a jmp.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 27 Aug 2024 00:53:54 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 6:

(2 comments)

I'm working on a test case, but here's updated code.

http://gerrit.cloudera.org:8080/#/c/21541/6/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

http://gerrit.cloudera.org:8080/#/c/21541/6/be/src/scheduling/scheduler.cc@601
PS6, Line 601:     if (a_hdfs_split.mtime() > b_hdfs_split.mtime()) return true;
> I think this is insufficient. If a.mtime < b.mtime and a.offset > b.offset,
Good point, changed to look like:
if (x.field != y.field) {
  return x.field > y.field;
}


http://gerrit.cloudera.org:8080/#/c/21541/6/be/src/scheduling/scheduler.cc@637
PS6, Line 637:       // To make this determinstic, break ties by comparing the instance idxs
> nit: deterministic
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 6
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Aug 2024 20:45:12 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16404/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Jun 2024 05:17:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Hello Kurt Deschler, Yida Wu, Michael Smith, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21541

to look at the new patch set (#2).

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................

IMPALA-12906: Incorporate scan range information into the tuple cache key

This change is accomplishing two things:
1. It incorporates scan range information into the tuple
   cache key.
2. It reintroduces deterministic scheduling as an option
   for mt_dop and uses it for HdfsScanNodes that feed
   into a TupleCacheNode.

The combination of these two things solves several problems:
1. When a table is modified, the list of scan ranges will
   change, and this will naturally change the cache keys.
2. When accessing a partitioned table, two queries may have
   different predicates on the partition columns. Since the
   predicates can be satisfied via partition pruning, they are
   not included at runtime. This means that two queries
   may have identical compile-time keys with only the scan
   ranges being different due to different partition pruning.
3. Each fragment instance processes different scan ranges, so
   each will have a unique cache key.

To incorporate scan range information, this introduces a new
per-fragment-instance cache key. At compile time, the planner
now keeps track of which HdfsScanNodes feed into a TupleCacheNode.
This is passed over to the runtime as a list of plan node ids
that contain scan ranges. At runtime, the fragment instance
walks through the list of plan nodes ids and hashes any scan ranges
associated with them. This hash is the per-fragment-instance
cache key. The combination of the compile-time cache key produced
by the planner and the per-fragment-instance cache key is a unique
identifier of the result.

Deterministic scheduling for mt_dop was removed via IMPALA-9655
with the introduction of the shared queue. This revives some of
the pre-IMPALA-9655 scheduling logic as an option. Since the
TupleCacheNode knows which HdfsScanNodes feed into it, the
TupleCacheNode turns on deterministic scheduling for all of those
HdfsScanNodes. Since this only applies to HdfsScanNodes that feed
into a TupleCacheNode, it means that any HdfsScanNode that doesn't
feed into a TupleCacheNode continues using the current algorithm.

Testing:
 - Added custom cluster tests for the scan range information
   including modifying a table, selecting from a partitioned
   table, and verifying that fragment instances have unique
   keys
 - Added basic frontend test to verify that deterministic scheduling
   gets set on the HdfsScanNode that feed into the TupleCacheNode.

Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/tuple-cache-node.cc
M be/src/exec/tuple-cache-node.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/common/ThriftSerializationCtx.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
15 files changed, 604 insertions(+), 67 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/21541/2
-- 
To view, visit http://gerrit.cloudera.org:8080/21541
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Hello Kurt Deschler, Yida Wu, Michael Smith, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21541

to look at the new patch set (#4).

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................

IMPALA-12906: Incorporate scan range information into the tuple cache key

This change is accomplishing two things:
1. It incorporates scan range information into the tuple
   cache key.
2. It reintroduces deterministic scheduling as an option
   for mt_dop and uses it for HdfsScanNodes that feed
   into a TupleCacheNode.

The combination of these two things solves several problems:
1. When a table is modified, the list of scan ranges will
   change, and this will naturally change the cache keys.
2. When accessing a partitioned table, two queries may have
   different predicates on the partition columns. Since the
   predicates can be satisfied via partition pruning, they are
   not included at runtime. This means that two queries
   may have identical compile-time keys with only the scan
   ranges being different due to different partition pruning.
3. Each fragment instance processes different scan ranges, so
   each will have a unique cache key.

To incorporate scan range information, this introduces a new
per-fragment-instance cache key. At compile time, the planner
now keeps track of which HdfsScanNodes feed into a TupleCacheNode.
This is passed over to the runtime as a list of plan node ids
that contain scan ranges. At runtime, the fragment instance
walks through the list of plan nodes ids and hashes any scan ranges
associated with them. This hash is the per-fragment-instance
cache key. The combination of the compile-time cache key produced
by the planner and the per-fragment-instance cache key is a unique
identifier of the result.

Deterministic scheduling for mt_dop was removed via IMPALA-9655
with the introduction of the shared queue. This revives some of
the pre-IMPALA-9655 scheduling logic as an option. Since the
TupleCacheNode knows which HdfsScanNodes feed into it, the
TupleCacheNode turns on deterministic scheduling for all of those
HdfsScanNodes. Since this only applies to HdfsScanNodes that feed
into a TupleCacheNode, it means that any HdfsScanNode that doesn't
feed into a TupleCacheNode continues using the current algorithm.

Testing:
 - Added custom cluster tests for the scan range information
   including modifying a table, selecting from a partitioned
   table, and verifying that fragment instances have unique
   keys
 - Added basic frontend test to verify that deterministic scheduling
   gets set on the HdfsScanNode that feed into the TupleCacheNode.

Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/tuple-cache-node.cc
M be/src/exec/tuple-cache-node.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/common/ThriftSerializationCtx.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
15 files changed, 611 insertions(+), 69 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/21541/4
-- 
To view, visit http://gerrit.cloudera.org:8080/21541
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.

Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21541/1/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
File fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java:

http://gerrit.cloudera.org:8080/#/c/21541/1/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java@125
PS1, Line 125:     return output.toString();
Should we print input scan nodes here?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 27 Jun 2024 18:48:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.

Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/21541/4/be/src/exec/tuple-cache-node.h
File be/src/exec/tuple-cache-node.h:

http://gerrit.cloudera.org:8080/#/c/21541/4/be/src/exec/tuple-cache-node.h@21
PS4, Line 21: #include <vector>
Why include vector in this header?


http://gerrit.cloudera.org:8080/#/c/21541/4/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

http://gerrit.cloudera.org:8080/#/c/21541/4/be/src/scheduling/scheduler.cc@600
PS4, Line 600:   bool operator<(InstanceAssignment& other) const { return weight > other.weight; }
Make this deterministic for equal weight case by comparing instance_idx.


http://gerrit.cloudera.org:8080/#/c/21541/4/be/src/scheduling/scheduler.cc@816
PS4, Line 816:             return ScanRangeWeight(a) > ScanRangeWeight(b);
Make this deterministic for equal weight


http://gerrit.cloudera.org:8080/#/c/21541/4/be/src/scheduling/scheduler.cc@821
PS4, Line 821:         pop_heap(instance_heap.begin(), instance_heap.end());
Kind of hard to follow why pop_heap/push_heap is being used here instead of just updating and sorting the vector. Also why isn't make_heap needed?


http://gerrit.cloudera.org:8080/#/c/21541/4/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
File fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java:

http://gerrit.cloudera.org:8080/#/c/21541/4/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java@128
PS4, Line 128:     output.append(detailPrefix + "input scan node ids: " +
If this list will typically be long, it should be split at keyFormatWidth



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 15 Aug 2024 15:09:27 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Hello Kurt Deschler, Yida Wu, Michael Smith, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21541

to look at the new patch set (#3).

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................

IMPALA-12906: Incorporate scan range information into the tuple cache key

This change is accomplishing two things:
1. It incorporates scan range information into the tuple
   cache key.
2. It reintroduces deterministic scheduling as an option
   for mt_dop and uses it for HdfsScanNodes that feed
   into a TupleCacheNode.

The combination of these two things solves several problems:
1. When a table is modified, the list of scan ranges will
   change, and this will naturally change the cache keys.
2. When accessing a partitioned table, two queries may have
   different predicates on the partition columns. Since the
   predicates can be satisfied via partition pruning, they are
   not included at runtime. This means that two queries
   may have identical compile-time keys with only the scan
   ranges being different due to different partition pruning.
3. Each fragment instance processes different scan ranges, so
   each will have a unique cache key.

To incorporate scan range information, this introduces a new
per-fragment-instance cache key. At compile time, the planner
now keeps track of which HdfsScanNodes feed into a TupleCacheNode.
This is passed over to the runtime as a list of plan node ids
that contain scan ranges. At runtime, the fragment instance
walks through the list of plan nodes ids and hashes any scan ranges
associated with them. This hash is the per-fragment-instance
cache key. The combination of the compile-time cache key produced
by the planner and the per-fragment-instance cache key is a unique
identifier of the result.

Deterministic scheduling for mt_dop was removed via IMPALA-9655
with the introduction of the shared queue. This revives some of
the pre-IMPALA-9655 scheduling logic as an option. Since the
TupleCacheNode knows which HdfsScanNodes feed into it, the
TupleCacheNode turns on deterministic scheduling for all of those
HdfsScanNodes. Since this only applies to HdfsScanNodes that feed
into a TupleCacheNode, it means that any HdfsScanNode that doesn't
feed into a TupleCacheNode continues using the current algorithm.

Testing:
 - Added custom cluster tests for the scan range information
   including modifying a table, selecting from a partitioned
   table, and verifying that fragment instances have unique
   keys
 - Added basic frontend test to verify that deterministic scheduling
   gets set on the HdfsScanNode that feed into the TupleCacheNode.

Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/tuple-cache-node.cc
M be/src/exec/tuple-cache-node.h
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/common/ThriftSerializationCtx.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java
M fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
15 files changed, 605 insertions(+), 67 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/21541/3
-- 
To view, visit http://gerrit.cloudera.org:8080/21541
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 3: Code-Review+1

(3 comments)

http://gerrit.cloudera.org:8080/#/c/21541/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

http://gerrit.cloudera.org:8080/#/c/21541/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1959
PS2, Line 1959:     // The reason we skip setting the deterministic scan range assignment field for
> Done
Ack


http://gerrit.cloudera.org:8080/#/c/21541/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@2065
PS2, Line 2065:       // To avoid polluting the explain output, only add this if mt_dop>0 and
> Done
Ack


http://gerrit.cloudera.org:8080/#/c/21541/2/tests/custom_cluster/test_tuple_cache.py
File tests/custom_cluster/test_tuple_cache.py:

http://gerrit.cloudera.org:8080/#/c/21541/2/tests/custom_cluster/test_tuple_cache.py@88
PS2, Line 88:   # Helper function to get a tuple cache metric from a single impalad.
> Right, this is just one impalad. Fixed.
Ack



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 29 Jun 2024 04:32:57 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 29 Jun 2024 05:34:14 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/21541/4/be/src/exec/tuple-cache-node.h
File be/src/exec/tuple-cache-node.h:

http://gerrit.cloudera.org:8080/#/c/21541/4/be/src/exec/tuple-cache-node.h@21
PS4, Line 21: #include <vector>
> Why include vector in this header?
Good point, that must be leftover from earlier iterations. Removed.


http://gerrit.cloudera.org:8080/#/c/21541/4/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

http://gerrit.cloudera.org:8080/#/c/21541/4/be/src/scheduling/scheduler.cc@600
PS4, Line 600:   bool operator<(InstanceAssignment& other) const { return weight > other.weight; }
> Make this deterministic for equal weight case by comparing instance_idx.
Done


http://gerrit.cloudera.org:8080/#/c/21541/4/be/src/scheduling/scheduler.cc@816
PS4, Line 816:             return ScanRangeWeight(a) > ScanRangeWeight(b);
> Make this deterministic for equal weight
Done


http://gerrit.cloudera.org:8080/#/c/21541/4/be/src/scheduling/scheduler.cc@821
PS4, Line 821:         pop_heap(instance_heap.begin(), instance_heap.end());
> Kind of hard to follow why pop_heap/push_heap is being used here instead of
This is the pre-IMPALA-9655 code that I brought back. All the instances start with zero work assigned and so the initial vector is already sorted, because it only cared about the weight and all the weights are the same. make_heap() is not needed for a sorted vector.

With the new code to make it deterministic, the comparison now cares about the instance idx. This still constructs it in a sorted order, so it doesn't require make_heap(). To make this clearer, I added a DCHECK to verify it is a heap via std::is_heap() and a comment about why it is already a heap.

This uses a heap, because we only care about finding the instance with the least work assigned. We don't need the vector to be fully sorted. A heap is O(log(n)) for each push/pop. Fully sorting the vector would be more expensive.


http://gerrit.cloudera.org:8080/#/c/21541/4/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
File fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java:

http://gerrit.cloudera.org:8080/#/c/21541/4/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java@128
PS4, Line 128:     output.append(detailPrefix + "input scan node ids: " +
> If this list will typically be long, it should be split at keyFormatWidth
Right now, this will always be a single entry. When we add support for unions, it can have an entry for each branch of the union. It gets reset at an exchange boundary, so it doesn't accumulate up the plan tree. In future, replicated dimension tables could also cause multiple entries.

Since these are one-three digit integers, I think we will be fine without wrapping for now.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Aug 2024 19:38:43 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 8:

Added a test case for the ScanRangeComparator


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 8
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 26 Aug 2024 22:48:35 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 10:

> Patch Set 10: Patch Set 9 was rebased

I accidentally uploaded an earlier version, working on restoring to PS8.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 10
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 27 Aug 2024 22:57:49 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/21541/3/be/src/exec/hdfs-scan-node-base.h
File be/src/exec/hdfs-scan-node-base.h:

http://gerrit.cloudera.org:8080/#/c/21541/3/be/src/exec/hdfs-scan-node-base.h@826
PS3, Line 826: scan rang
> nit. scan range
Done


http://gerrit.cloudera.org:8080/#/c/21541/3/common/thrift/PlanNodes.thrift
File common/thrift/PlanNodes.thrift:

http://gerrit.cloudera.org:8080/#/c/21541/3/common/thrift/PlanNodes.thrift@354
PS3, Line 354: scan rang
> nit. scan range
Done


http://gerrit.cloudera.org:8080/#/c/21541/3/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
File fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java:

http://gerrit.cloudera.org:8080/#/c/21541/3/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java@115
PS3, Line 115:     output.append(detailPrefix + "cache key: " + compileTimeKey_ + "\n");
> Should be better to use append.
For code that isn't performance sensitive, I think we should optimize for readability. I prefer the simpler + syntax or String.format() unless we really need the performance.


http://gerrit.cloudera.org:8080/#/c/21541/3/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java@124
PS3, Line 124: output.append(detailPrefix + "[" + hashTrace_.substring(idx, stop_idx) + "]\n");
> It seems that using append would be better than using the '+', as '+' creat
Same as other location.


http://gerrit.cloudera.org:8080/#/c/21541/3/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java@128
PS3, Line 128:     output.append(detailPrefix + "input scan node ids: " +
             :         String.join(",", input_scan_node_ids_strs) + "\n");
> Should be better to use append.
Same as other location.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 31 Jul 2024 17:28:08 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16798/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 8
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 26 Aug 2024 23:18:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has uploaded a new patch set (#9) to the change originally created by Joe McDonnell. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................

IMPALA-12906: Incorporate scan range information into the tuple cache key

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/21541/9
--
To view, visit http://gerrit.cloudera.org:8080/21541
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 9
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.

Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/21541/3/be/src/exec/hdfs-scan-node-base.h
File be/src/exec/hdfs-scan-node-base.h:

http://gerrit.cloudera.org:8080/#/c/21541/3/be/src/exec/hdfs-scan-node-base.h@826
PS3, Line 826: scanrange
nit. scan range


http://gerrit.cloudera.org:8080/#/c/21541/3/common/thrift/PlanNodes.thrift
File common/thrift/PlanNodes.thrift:

http://gerrit.cloudera.org:8080/#/c/21541/3/common/thrift/PlanNodes.thrift@354
PS3, Line 354: scanrange
nit. scan range


http://gerrit.cloudera.org:8080/#/c/21541/3/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java
File fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java:

http://gerrit.cloudera.org:8080/#/c/21541/3/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java@115
PS3, Line 115:     output.append(detailPrefix + "cache key: " + compileTimeKey_ + "\n");
Should be better to use append.
output.append(detailPrefix).append("cache key: ").append(compileTimeKey_).append("\n");


http://gerrit.cloudera.org:8080/#/c/21541/3/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java@124
PS3, Line 124: output.append(detailPrefix + "[" + hashTrace_.substring(idx, stop_idx) + "]\n");
It seems that using append would be better than using the '+', as '+' creates an intermediate String. Even though it is not changed in this patch, it might be better to optimize it as well.


http://gerrit.cloudera.org:8080/#/c/21541/3/fe/src/main/java/org/apache/impala/planner/TupleCacheNode.java@128
PS3, Line 128:     output.append(detailPrefix + "input scan node ids: " +
             :         String.join(",", input_scan_node_ids_strs) + "\n");
Should be better to use append.
output.append(detailPrefix).append("input scan node ids: ") .append(String.join(",", input_scan_node_ids_strs)) .append("\n");



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 18 Jul 2024 17:16:16 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/21541/2/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:

http://gerrit.cloudera.org:8080/#/c/21541/2/be/src/exec/hdfs-scan-node-base.cc@488
PS2, Line 488:     deterministic_scanrange_assignment_(hdfs_scan_node.deterministic_scanrange_assignment),
line too long (91 > 90)


http://gerrit.cloudera.org:8080/#/c/21541/2/tests/custom_cluster/test_tuple_cache.py
File tests/custom_cluster/test_tuple_cache.py:

http://gerrit.cloudera.org:8080/#/c/21541/2/tests/custom_cluster/test_tuple_cache.py@307
PS2, Line 307: "
flake8: E501 line too long (92 > 90 characters)


http://gerrit.cloudera.org:8080/#/c/21541/2/tests/custom_cluster/test_tuple_cache.py@313
PS2, Line 313: "
flake8: E501 line too long (92 > 90 characters)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 28 Jun 2024 23:41:10 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 2:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/21541/2/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:

http://gerrit.cloudera.org:8080/#/c/21541/2/be/src/exec/hdfs-scan-node-base.cc@488
PS2, Line 488:     deterministic_scanrange_assignment_(hdfs_scan_node.deterministic_scanrange_assignment),
> line too long (91 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/21541/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

http://gerrit.cloudera.org:8080/#/c/21541/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1959
PS2, Line 1959:     // There reason we skip setting the deterministic scan range assignment field for
> nit: "The" not "There"
Done


http://gerrit.cloudera.org:8080/#/c/21541/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@2065
PS2, Line 2065:       // To avoid polluting the explain output, only add this if this is mt_dop>0 and
> nit: "this" is used in two different ways here. I'd shorten it to "only add
Done


http://gerrit.cloudera.org:8080/#/c/21541/2/tests/custom_cluster/test_tuple_cache.py
File tests/custom_cluster/test_tuple_cache.py:

http://gerrit.cloudera.org:8080/#/c/21541/2/tests/custom_cluster/test_tuple_cache.py@88
PS2, Line 88:   # Get cache metrics from all impalads
> This comment seems wrong, it's just getting it from a single impalad.
Right, this is just one impalad. Fixed.


http://gerrit.cloudera.org:8080/#/c/21541/2/tests/custom_cluster/test_tuple_cache.py@307
PS2, Line 307: "
> flake8: E501 line too long (92 > 90 characters)
Done


http://gerrit.cloudera.org:8080/#/c/21541/2/tests/custom_cluster/test_tuple_cache.py@313
PS2, Line 313: "
> flake8: E501 line too long (92 > 90 characters)
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 29 Jun 2024 00:05:15 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.

Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 4: Code-Review+1

LGTM! I tested running the patch with all the tpch and tpcds queries and tuple cache enabled, didn't see any related issues. Wait to see if Kurt has any more comments, if not, we can probably move ahead.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 09 Aug 2024 02:54:03 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12906: Incorporate scan range information into the tuple cache key

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21541 )

Change subject: IMPALA-12906: Incorporate scan range information into the tuple cache key
......................................................................


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/21541/6/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

http://gerrit.cloudera.org:8080/#/c/21541/6/be/src/scheduling/scheduler.cc@601
PS6, Line 601:     const HdfsFileSplitPB& a_hdfs_split = a.scan_range().hdfs_file_split();
> Good point, changed to look like:
Ack


http://gerrit.cloudera.org:8080/#/c/21541/6/be/src/scheduling/scheduler.cc@637
PS6, Line 637: /// Helper class used in CreateScanInstances() to track the amount of work assigned
> Done
Ack


http://gerrit.cloudera.org:8080/#/c/21541/7/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

http://gerrit.cloudera.org:8080/#/c/21541/7/be/src/scheduling/scheduler.cc@593
PS7, Line 593:   if (a_weight != b_weight) {
nit: Could be a one-liner. As could the assignment above.


http://gerrit.cloudera.org:8080/#/c/21541/7/be/src/scheduling/scheduler.cc@603
PS7, Line 603:     if (a_hdfs_split.mtime() != b_hdfs_split.mtime()) {
If these get more expensive to compute, do we really want to repeat them for every expression?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe298fff0f644ce931a2aa934ebb98f69aab9d34
Gerrit-Change-Number: 21541
Gerrit-PatchSet: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Aug 2024 20:58:47 +0000
Gerrit-HasComments: Yes