Dan Burkert has posted comments on this change. Change subject: Add ColumnPredicate class ......................................................................
Patch Set 2: (25 comments) http://gerrit.cloudera.org:8080/#/c/2137/2/src/kudu/common/column_predicate-test.cc File src/kudu/common/column_predicate-test.cc: Line 30: void test_merge(const ColumnSchema& column, > - style: TestMerge Done Line 48: ASSERT_EQ(expected.predicate_type(), type); > since the user passes in expected, why do you also need to pass in 'type'? Since the result of the range constructor can be Range, Equality, or None, passing the type in explicitly is necessary to make sure the result of the range constructor is as you expect. Line 58: // [--------) > can you add 'AND' at the end of this line? Done Line 60: // > and a '=" or "transforms to:" here? Done Line 225: // Test that the range constructor handles equality and disjoint ranges. > I think you mean 'degenerate' ranges, not disjoint? Done http://gerrit.cloudera.org:8080/#/c/2137/2/src/kudu/common/column_predicate.cc File src/kudu/common/column_predicate.cc: Line 95: predicate_type_ = PredicateType::None; > nit: indent Done Line 112: default: LOG(FATAL) << "unknown predicate type"; > is it better to leave this out and put the LOG(FATAL) after the switch? tha Done Line 117: CHECK(predicate_type_ == PredicateType::Range); > CHECK_EQ (or does that fail because of inability to stringify an enum class yes, it fails to compile with ostream issues. Line 125: > remove blank line (or maybe put the blank line above the 'case' line if you Done Line 128: && (lower_ == nullptr || column_.type_info()->Compare(lower_, other.lower_) < 0)) { > nit: usually put && on previous line Done Line 138: // Check if the resulting range is disjoint or an equality. > I think 'empty' is better than 'disjoint' (disjoint to me implies two separ Done Line 154: || column_.type_info()->Compare(upper_, other.lower_) <= 0) { > nit: || on prev line Done Line 274: return strings::Substitute("(`$0` BETWEEN $1 AND $2)", > this isn't quite right since BETWEEN is an inclusive range Done Line 288: CHECK(column_.Equals(other.column_, false)); > seems fishy to CHECK here instead of just returning false Done http://gerrit.cloudera.org:8080/#/c/2137/2/src/kudu/common/column_predicate.h File src/kudu/common/column_predicate.h: Line 50: // so it's lifetime must be managed to make sure it does not reference invalid > nit: its Done Line 59: static ColumnPredicate Equality(ColumnSchema column, const void* value); > sort of odd that this class is partially managing memory -- it copies the Yah, I originally wrote the class to just hold a TypeInfo* instead of the full ColumnSchema, but it ended up not playing well with some places in our codebase that assume the predicate has a reference to its column name. Even though the memory management is a bit convoluted, the state of the world is already such that a scan holds an arena with a bunch of memory for these things, so it's natural that this works the same way. Line 62: // exclusive upper bound. > clarify if you can pass NULLs to represent open-ended intervals Done Line 71: // arena must outlive the returned predicate. > should clarify what the arena is used for Done Line 73: // If a normalized column predicate can not be created, then none will be > nit: 'cannot' Done Line 96: // Data is not copied from the other predicate, so it's data must continue > "its" Done Line 99: // If the predicates are disjoint, then an IllegalArgument status is returned. > not true anymore Done Line 115: void Evaluate(const ColumnBlock& block, SelectionVector *sel) const; > style nit: * goes with the type Done Line 165: // The exclusive upper bound value if this is a Range predicate. > what about open-ended intervals? They are transformed into normal [closed, open) bounds. Line 167: }; > should you DISALLOW_COPY_AND_ASSIGN? if not, then why do you need SetToNone SetToNone is easier since it doesn't have to muck with the column. I've made SetToNone and the None ctor function private since they really shouldn't be used (None predicates shouldn't be created except via the result of a Merge). Line 169: // Compares predicates according to selectivity. > should note that in this context, a higher 'selectivity' means an estimatio I actually meant the opposite, that an Equals predicate is more selective (higher selectivity ) than a Range predicate. Should I flip it? -- To view, visit http://gerrit.cloudera.org:8080/2137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I72cac35d7a168f96c2ee52afd284489e99755e3f Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
