Mahesh Reddy 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 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/16674/8/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/16674/8/src/kudu/common/partition.h@267 PS8, Line 267: void IsSingleColumnHashParition(const Schema& schema, nit: probably better to declare this function in the header file and define it in 'partition.cc'. So just move the function body to the source file. http://gerrit.cloudera.org:8080/#/c/16674/8/src/kudu/common/partition.h@266 PS8, Line 266: // Set is_hash_partition to true if col_name is one of hash partition schema. : void IsSingleColumnHashParition(const Schema& schema, : const std::string& col_name, : bool* is_hash_partition) const { > It'd be better to return bool instead of using as an out parameter. +1 http://gerrit.cloudera.org:8080/#/c/16674/8/src/kudu/common/partition.h@273 PS8, Line 273: if (!s.ok()) Shouldn't this check be directly below the call to FindColumn()? What happens if the column is not found and we call column_id()? http://gerrit.cloudera.org:8080/#/c/16674/8/src/kudu/common/scan_spec.cc File src/kudu/common/scan_spec.cc: http://gerrit.cloudera.org:8080/#/c/16674/8/src/kudu/common/scan_spec.cc@186 PS8, Line 186: Status s = schema.FindColumn(col_name, &idx); : if (!s.ok()) continue; nit: can these lines be merged? same elsewhere with using temp variable for Status object on one line and checking on a separate line. http://gerrit.cloudera.org:8080/#/c/16674/8/src/kudu/common/scan_spec.cc@193 PS8, Line 193: col_name, > nit: we've already found the column index -- could we pass 'idx' or the col +1. If we pass 'idx' here, we would save an extra call to FindColumn() in 'IsSingleColumnHashPartition()'. -- 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: 8 Gerrit-Owner: wangning <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: wangning <[email protected]> Gerrit-Comment-Date: Tue, 03 Nov 2020 01:06:06 +0000 Gerrit-HasComments: Yes
