Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24578 )
Change subject: KUDU-3800 Add diff-scan option to return unobservable rows ...................................................................... Patch Set 15: (9 comments) Thank you for cleaning the client API and adding extra test scenarios! Overall PS15 looks good to me, just a few nits and a couple of (presumably) AI slops to fix. http://gerrit.cloudera.org:8080/#/c/24578/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24578/15//COMMIT_MSG@32 PS15, Line 32: SetDiffScan overload takes a : SetDiffScan takes a nit: seems to be a duplication? http://gerrit.cloudera.org:8080/#/c/24578/15//COMMIT_MSG@56 PS15, Line 56: The three argument SetDiffScan() overloads on KuduScanner and : KuduScanTokenBuilder are exported for user code but documented private. The prior : two argument overloads were KUDU_NO_EXPORT (never part of the client ABI) and : are removed. nit: as of PS15, the only SetDiffScan() left has three parameters (or maybe I'm missing something) http://gerrit.cloudera.org:8080/#/c/24578/15/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/24578/15/src/kudu/client/scan_token-test.cc@667 PS15, Line 667: visibility == KuduScanner::INCLUDE_UNOBSERVABLE : ? kudu::INCLUDE_UNOBSERVABLE : kudu::OBSERVABLE_ONLY nit: create an utility function out of this and use here and elsewhere (at least, at line 685)? http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/tablet/diff_scan-test.cc File src/kudu/tablet/diff_scan-test.cc: http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/tablet/diff_scan-test.cc@520 PS12, Line 520: > Done - Added OrderedDiffScanWithDeletesTest.MixedRowLifecycles to cover the Thank you for adding those new scenarios! http://gerrit.cloudera.org:8080/#/c/24578/15/src/kudu/tablet/diff_scan-test.cc File src/kudu/tablet/diff_scan-test.cc: http://gerrit.cloudera.org:8080/#/c/24578/15/src/kudu/tablet/diff_scan-test.cc@566 PS15, Line 566: kKey2 stores as -2 Per line 536 of PS15, kKey2 is 2, not -2, isn't it? http://gerrit.cloudera.org:8080/#/c/24578/15/src/kudu/tablet/diff_scan-test.cc@566 PS15, Line 566: → nit for here and elsewhere: please use only ASCII characters in the source files where it's possible to avoid using unicode (add this as a rule for AI agents?) http://gerrit.cloudera.org:8080/#/c/24578/15/src/kudu/tablet/diff_scan-test.cc@567 PS15, Line 567: EXPECT_STR_CONTAINS Any particular reason behind using EXPECT_xxx instead of ASSERT_xxx here and below? http://gerrit.cloudera.org:8080/#/c/24578/15/src/kudu/tablet/diff_scan-test.cc@577 PS15, Line 577: -2 (kKey2) Per line 536 of PS15, kKey2 is 2, not -2, isn't it? Also, it would be nice to add a comment to explain this non-obvious sorted order. http://gerrit.cloudera.org:8080/#/c/24578/15/src/kudu/tablet/diff_scan-test.cc@598 PS15, Line 598: static void DoUnorderedScan( : std::shared_ptr<Tablet> tablet, : MvccSnapshot snap_to_exclude, : MvccSnapshot snap_to_include, : RowVisibility row_visibility, : vector<string>* rows) { : RowIteratorOptions opts; : opts.snap_to_exclude = std::move(snap_to_exclude); : opts.snap_to_include = std::move(snap_to_include); : opts.order = UNORDERED; : opts.include_deleted_rows = true; : opts.row_visibility = row_visibility; : static constexpr bool kIsDeletedDefault = false; : SchemaBuilder builder(*tablet->metadata()->schema()); : ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() : .name("deleted") : .type(IS_DELETED) : .read_default(&kIsDeletedDefault))); : Schema projection = builder.BuildWithoutIds(); : opts.projection = &projection; : unique_ptr<RowwiseIterator> row_iterator; : ASSERT_OK(tablet->NewRowIterator(std::move(opts), &row_iterator)); : ASSERT_TRUE(row_iterator); : ScanSpec spec; : ASSERT_OK(row_iterator->Init(&spec)); : ASSERT_OK(tablet::IterateToStringList(row_iterator.get(), rows)); : } Any chance of unifying this and OrderedDiffScanWithDeletesTest::DoScan()? If there isn't a common base, maybe separate this as a stand-alone utility function since both are static methods anyway? -- To view, visit http://gerrit.cloudera.org:8080/24578 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0d0727fb2b89c727c669ec218e3bb8594336359a Gerrit-Change-Number: 24578 Gerrit-PatchSet: 15 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Fri, 28 Aug 2026 06:43:16 +0000 Gerrit-HasComments: Yes
