David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1880 Prevent eager ignoring of NULLs ......................................................................
Patch Set 6: (10 comments) http://gerrit.cloudera.org:8080/#/c/6029/6//COMMIT_MSG Commit Message: PS6, Line 20: Additional tablet unit tests are added to cover the IsNull case with : NULL values at the beginning of columns for various types. mention that the tests fail without the fix and pass with it http://gerrit.cloudera.org:8080/#/c/6029/6/src/kudu/common/column_materialization_context.h File src/kudu/common/column_materialization_context.h: PS6, Line 79: NotEvaluatingIsNull If I understand correctly, you need to be able to tell whether there is an "is null" predicate so that you don't clear the null bits. could this be EvaluatingIsNull()? and then on the call site you'd do if (!EvaluatingIsNull) the body would be: pred_ && pred_->predicate_type() == PredicateType::IsNull http://gerrit.cloudera.org:8080/#/c/6029/6/src/kudu/tablet/all_types-scan-correctness-test.cc File src/kudu/tablet/all_types-scan-correctness-test.cc: PS6, Line 12: BothRowOps how about RowOpsBase? (for instance I'll likely want to add a Timestamp test) PS6, Line 79: don't need the extra space anymore (since c++11) here and everywhere else PS6, Line 98: comma after "repeated" and after "non-null" PS6, Line 133: AllTypesScanCorrectnessTest thanks for doing this. it looks soooo much better. PS6, Line 133: class AllTypesScanCorrectnessTest : public KuduTabletTest { could you add a summary of the test to the class header? (not too much detail, just the gist) PS6, Line 254: AUTO_ENCODING we should likely be explicit (i.e set the encoding that AUTO_ENCODING resolves to) in tests. also likely have all possible encodings (plain, bitshuffle, rle) for all numeric types PS6, Line 283: spaces (two) not tabs PS6, Line 284: same -- To view, visit http://gerrit.cloudera.org:8080/6029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib418a4fcc2794ce2f686e864f51834fb4fb8b048 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
