Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12109 )
Change subject: tablet: add support for diff scan iterator ...................................................................... Patch Set 1: (12 comments) http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/common/schema.cc@387 PS1, Line 387: if (!col.has_read_default()) { : return Status::InvalidArgument(Substitute("Virtual column $0 $1 must have a default value", : col.name(), col.TypeToString())); : } > Why is this important? What should the invariant be for virtual columns? Fo Good question. It didn't work without a default, but that's something I'm going to have to spend a little more time understanding. For now I am going to kick this can down the road and remove this change from this patch. http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/common/schema.cc@392 PS1, Line 392: TODO(mpercy): Is this safe? > Seems OK. max_col_id_ is immutable in Schema once constructed, so there's n Thanks for taking a look. http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/tablet/diff_scan-test.cc File src/kudu/tablet/diff_scan-test.cc: http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/tablet/diff_scan-test.cc@18 PS1, Line 18: #include <stdint.h> > Nit: cstdint Done http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/tablet/diff_scan-test.cc@58 PS1, Line 58: tablet::MvccSnapshot snap1; > You're in kudu::tablet::, so why do you need this prefix? Done http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/tablet/diff_scan-test.cc@70 PS1, Line 70: // 3. Reinsert the rows and flush the MRS. > Nit: It's not really a REINSERT though; it's just an INSERT into a new MRS Ack http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/tablet/diff_scan-test.cc@75 PS1, Line 75: std::vector<std::string> rows; > using Done http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/tablet/diff_scan-test.cc@96 PS1, Line 96: ASSERT_OK(tablet->NewRowIterator(projection, > If you expose RowIteratorOptions at this level, you could expose the ghosts Good suggestion; I was hesitant to plumb it all the way up in order to hide it a little better, but it gets rid of the need to magically determine when include_deleted_rows should be set, so it's worth exposing. http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/tablet/tablet.h@199 PS1, Line 199: // Create a new row iterator for some historical snapshot. > Update the doc. Would be good to specify why we aren't using RowIteratorOpt RowIteratorOptions has pointers to fields that outlast the lifetime of this method and potentially the caller. I'm holding those objects in Tablet::Iterator which makes sense from a lifetime perspective but makes using the options as input a little awkward. That said, I think it's reasonable and with a little documentation should be sufficiently comprehensible. http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/tablet/tablet.h@575 PS1, Line 575: member > Specifically, just the pointers in 'opts'. Everything else is a value type. Done http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/tablet/tablet.h@795 PS1, Line 795: Schema schema_; > Nit: why rename from projection_? It's more appropriate given this is an it Fair point - I'll leave it as it was. http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/tablet/tablet.cc@420 PS1, Line 420: RowIteratorOptions opts; > Need to also set an IOContext, no? I'll make this part of the contract for NewRowIterator() and the Tablet::Iterator constructor http://gerrit.cloudera.org:8080/#/c/12109/1/src/kudu/tablet/tablet.cc@428 PS1, Line 428: for (const auto& c : projection.columns()) { > You can use Schema::find_first_is_deleted_virtual_column. Moved this decision logic -- To view, visit http://gerrit.cloudera.org:8080/12109 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9aa0ce2276bdd37688e7a91a2efcf49a8f802eb5 Gerrit-Change-Number: 12109 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 20 Dec 2018 07:57:49 +0000 Gerrit-HasComments: Yes
