Andrew Wong has posted comments on this change. Change subject: KUDU-1893 Ensure evaluation of added columns ......................................................................
Patch Set 2: (13 comments) http://gerrit.cloudera.org:8080/#/c/6129/2/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: PS2, Line 497: // Cases where the selection vector can be cleared: : // 1. There is a read_default and it does not satisfy the predicate : // 2. There is no read_default and the predicate is search fo NULLs > move this below the next if block Done PS2, Line 499: is search fo NULLs > rephrase/typo Done PS2, Line 506: DecoderEvalNotDisabled > not your fault, but this means DecoderEvalEnabled, right? we should rename Actually nope! Decoder-level evaluation flag has three states: disabled, enabled, and not-set. Some code will run if it's not-set or enabled (i.e. not disabled), some code will be run when disabled, so there is also DecoderEvalNotSupported. This one i could see renaming to DecoderEvalDisabled, but my reasoning for writing it this way was that some blocks just don't support evaluation, and this is less in-line with "disabling" (e.g. shutting it off with a flag) and more in-line with a lack of support. Line 509: sel_view.ClearBits(dst->nrows()); > Is it safe to also skip copying the data if it's just being filtered? Seem Right, since these values are not going to be used later, it should be safe. PS2, Line 506: if (ctx->DecoderEvalNotDisabled() && : !ctx->pred()->EvaluateCell(typeinfo_->physical_type(), value_)) { : SelectionVectorView sel_view(ctx->sel()); : sel_view.ClearBits(dst->nrows()); : } > no need to set cell values if value doesnt match the predicate, right? Right, since these values are not going to be used later, it should be safe. http://gerrit.cloudera.org:8080/#/c/6129/2/src/kudu/common/column_predicate.cc File src/kudu/common/column_predicate.cc: PS2, Line 599: type; > get the type name (from the type info) Done http://gerrit.cloudera.org:8080/#/c/6129/2/src/kudu/common/column_predicate.h File src/kudu/common/column_predicate.h: PS2, Line 199: EvaluateCell > docs Done http://gerrit.cloudera.org:8080/#/c/6129/2/src/kudu/tablet/tablet-scan-correctness-test.cc File src/kudu/tablet/tablet-scan-correctness-test.cc: PS2, Line 116: result count to counter > "result count in 'counter'" Done PS2, Line 116: > missing "a" Done PS2, Line 413: class AlteredTabletCorrectnessTest : public TabletCorrectnessTest > is the problem just on int32 or on other types too? is there anything to ga The issue isn't specific to int32, I just chose int32 for this test since the original bug was filed for an int32 column. I think it would be a nice check to do more types. I think the only one that has a different code path is BINARY types, so having at least a string test would be good to include. On the same note, I agree it'd be especially good to test decoder-level evaluation. PS2, Line 422: void AlterTable(const void* read_default, const void* write_default) { > blank line above this method, docs Done PS2, Line 454: CheckResults > doc this Done PS2, Line 512: This should yield the same results as the query on : // val_b where val_c is not null. > can you make sure the results match then? Yep! Since each run has multiple scans, I left all the checks to the very end with CheckResults(). I'll leave them to there for now since I think there is value in running through the entire roster of scans before asserting a failure. -- To view, visit http://gerrit.cloudera.org:8080/6129 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic10919962d11effbb1b66d97660acd012b6b4be9 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
