Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16674 )
Change subject: KUDU-1644 hash-partition based in-list predicate optimization ...................................................................... Patch Set 17: (5 comments) http://gerrit.cloudera.org:8080/#/c/16674/17/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/16674/17/src/kudu/common/partition.cc@458 PS17, Line 458: CHECK_EQ(partition.hash_buckets().size(), hash_bucket_schemas_.size()); nit: can you also add this as a DCHECK_EQ() to HashPartitionContainsRowImpl? I found it helpful in orienting myself with this code, to understand the relationship between hash_buckets() and this partition schema. http://gerrit.cloudera.org:8080/#/c/16674/17/src/kudu/common/partition.cc@460 PS17, Line 460: const HashBucketSchema& hash_bucket_schema = hash_bucket_schemas_[i]; : int32_t bucket = -1; : RETURN_NOT_OK(BucketForRow(row, hash_bucket_schema, &bucket)); : : if (bucket != partition.hash_buckets()[i]) { : *contains = false; : return Status::OK(); : } nit: could we use HashPartitionContainsRow() here instead, to avoid the code duplication? http://gerrit.cloudera.org:8080/#/c/16674/17/src/kudu/common/scan_spec.h File src/kudu/common/scan_spec.h: http://gerrit.cloudera.org:8080/#/c/16674/17/src/kudu/common/scan_spec.h@80 PS17, Line 80: Supports hash schema with only onekey. nit: maybe rephrase this as "Only supports pruning for single-column hash schemas." http://gerrit.cloudera.org:8080/#/c/16674/17/src/kudu/common/scan_spec.cc File src/kudu/common/scan_spec.cc: http://gerrit.cloudera.org:8080/#/c/16674/17/src/kudu/common/scan_spec.cc@323 PS17, Line 323: && lower_bound_key_ != nullptr) { Was this required for this patch? http://gerrit.cloudera.org:8080/#/c/16674/14/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/16674/14/src/kudu/tserver/tablet_service.cc@2663 PS14, Line 2663: if (spec.IsInListPredicateValuesEmpty()) { : // Return if hash predicate values is empty. : *has_more_results = false; : return Status::OK(); : } Looking through the failed test logs, TabletServerTest.TestNonPositiveLimitsShortCircuit should pass if we remove this, since we call it at L2732 anyway. The below lines L2668-L2724 must run for that test to pass because result_collector->InitSerializer() is required to correctly initialize the response data. -- To view, visit http://gerrit.cloudera.org:8080/16674 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I202001535669a72de7fbb9e766dbc27db48e0aa2 Gerrit-Change-Number: 16674 Gerrit-PatchSet: 17 Gerrit-Owner: wangning <1994wangn...@gmail.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: wangning <1994wangn...@gmail.com> Gerrit-Comment-Date: Fri, 06 Nov 2020 07:25:08 +0000 Gerrit-HasComments: Yes
