Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/23074 )
Change subject: IMPALA-13898: Incorporate partition information into tuple cache keys ...................................................................... Patch Set 10: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/23074/10/be/src/exec/tuple-cache-node.cc File be/src/exec/tuple-cache-node.cc: http://gerrit.cloudera.org:8080/#/c/23074/10/be/src/exec/tuple-cache-node.cc@503 PS10, Line 503: id_to_scan_node_map[exec_node->plan_node().tnode_->node_id] = scan_node; It seems the exec_node->plan_node().tnode_->node_id are unique here, would it be good if we add a DCHECK to ensure it? "int node_id = exec_node->plan_node().tnode_->node_id; DCHECK(id_to_scan_node_map.find(node_id) == id_to_scan_node_map.end()) << "Duplicate node_id detected: " << node_id;" http://gerrit.cloudera.org:8080/#/c/23074/10/be/src/exec/tuple-cache-node.cc@521 PS10, Line 521: if (partition_desc != nullptr) { I think this is only used for release version when the previous DCHECK doesn't work there, do we need to also put some warning logging if it is nullptr? http://gerrit.cloudera.org:8080/#/c/23074/10/tests/custom_cluster/test_tuple_cache.py File tests/custom_cluster/test_tuple_cache.py: http://gerrit.cloudera.org:8080/#/c/23074/10/tests/custom_cluster/test_tuple_cache.py@422 PS10, Line 422: assert result1.data[0].split("\t") == ["1", "1"] Is it good if we also check the length? "assert len(result1.data) == 1" http://gerrit.cloudera.org:8080/#/c/23074/10/tests/custom_cluster/test_tuple_cache.py@430 PS10, Line 430: assert result2.data[0].split("\t") == ["1", "2"] Is it good if we also check the length? "assert len(result2.data) == 1" -- To view, visit http://gerrit.cloudera.org:8080/23074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a7109fcf8a30bf915bb566f7d642f8037793a8c Gerrit-Change-Number: 23074 Gerrit-PatchSet: 10 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Tue, 15 Jul 2025 22:45:58 +0000 Gerrit-HasComments: Yes
