Dan Burkert has posted comments on this change.

Change subject: KUDU-1363: Add IN-list predicate type
......................................................................


Patch Set 15:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/2986/12/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:

Line 546:   }
> consider adding a test cases for
This actually isn't possible since the KuduValue API has no way to create a 
null value.


http://gerrit.cloudera.org:8080/#/c/2986/12/src/kudu/client/scan_predicate.cc
File src/kudu/client/scan_predicate.cc:

Line 122:   vals_list.reserve(vals_.size());
> vals_list.reserve(vals_.size()) ?
Done


http://gerrit.cloudera.org:8080/#/c/2986/12/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:

PS12, Line 334:         return this->column_.type_info()->Compare(lhs, rhs) < 0;
              :       };
              : 
              :  
> an optimization opportunity if we expect IN() lists to be large.  Get the u
I've added some optimizations around first/last value ranges, let me know if 
you think there is more to do.


Line 352:       return;
> What if it was IN (NULL) predicate?
IN (NULL) is not possible, since KuduValue can't be a null value.


PS12, Line 501:     case UINT16: return EvaluateForPhysicalType<UINT16>(block, 
sel);
              :     case UINT32: return EvaluateForPhysicalType<UINT32>(block, 
sel);
              :     case UINT64: return EvaluateForPhysicalType<UINT64>(block, 
sel);
              :     case FLOAT: return EvaluateForPhysicalType<FLOAT>(block, 
sel);
              :     case DOUBLE: return EvaluateForPhysicalType<DOUBLE>(block, 
sel);
              :     case BINARY: return EvaluateForPhysicalType<BINARY>(block, 
sel);
              :     default: LOG(FATAL) << "unknown physical type: " << 
block.type_info()->physical_type();
              :   }
              : }
              : 
              : string ColumnPredicate::ToString() const {
              :   switch (predicate_type()) {
              :     case PredicateType::None: return strings::Substitute("`$0` 
NONE", column_.name());
              :     case PredicateType::Range: {
              :    
> May be, it's time to start using the switch() here instead?
Done


PS12, Line 537: 
> Is this by-copy capture?  Consider replacing with by-reference capture to a
Done


PS12, Line 532:     case PredicateType::InList: {
              :       string ss = "`";
              :       ss.append(column_.name());
              :       ss.append("` IN (");
              :       bool is_first = true;
              :       for (auto* value : values_) {
              :         if (is_first) {
              :  
> This looks strange to me: given the name of the method, I would expect it t
I've removed this since it was only used in a single place, and I agree that 
the name could be misleading.


http://gerrit.cloudera.org:8080/#/c/2986/12/src/kudu/common/key_util.cc
File src/kudu/common/key_util.cc:

PS12, Line 154:     size_t size = column.type_info()->size();
              :     switch (predicate->predicate_type()) {
              :       case PredicateType::Equality:
              :         memcpy(row->mutable_cell_ptr(*col_idx_it), 
predicate->raw_lower(), size);
              :         pushed_predicates++;
              :         final_predicate = predicate;
              :         break;
              :       case PredicateType::Range:
              :         if (predicate->raw_upper() != nullptr) {
              :           memcpy(row->mutable_cell_ptr(*col_idx_it), 
predicate->raw_upper(), size);
              :           pushed_predicates++;
              :           final_predicate = predicate;
              :         }
              :         // After the first column with a range constraint we 
stop pushing
              :         // constraints into the upper bound. Instead, we push 
minimum values
              :         // to the remaining columns (below), which is the 
maximally tight
              :         // constraint.
              :         break_loop = true;
              :         break;
              :       case PredicateType::IsNotNull:
              :         break_loop = true;
              :         break;
              :       case PredicateType::InList:
              :         // Since the InList predicate is a sorted vector of 
values, the last
              :    
> nit: consider converting this into switch()
Done in a prequel patch.


PS12, Line 215:   // Step 1: copy predicates into the row in key column order, 
stopping after
              :   // the first missing predicate.
              : 
              :   bool break_loop = false;
              :   for (auto col_idx_it = first; !break_loop && col_idx_it < 
last; std::advance(col_idx_it, 1)) {
              :     const ColumnSchema& column = schema.column(*col_idx_it);
              :     const ColumnPredicate* predicate = FindOrNull(predicates, 
column.name());
              :     if (predicate == nullptr) break;
              :     size_t size = column.type_info()->size();
              : 
              :     switch (predicate->predicate_type()) {
              :       case PredicateType::Range:
              :         if (predicate->raw_lower() == nullptr) {
              :           break_loop = true;
              :           break;
              :         }
              :         // Fall through.
              :       case PredicateType::Equality:
              :    
> nit: consider converting this into switch()
Done in a prequel patch.


http://gerrit.cloudera.org:8080/#/c/2986/12/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:

PS12, Line 395: CopyPredicateBou
> Why is it necessary here?
woops, leftover.  removed.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar <abhyan...@gmail.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar <abhyan...@gmail.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to