Abhishek Chennaka 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 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/24578/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24578/15//COMMIT_MSG@20 PS15, Line 20: (and whose delete was propagated) > Did you mean 'not' propagated? Better to just remove this line to avoid any Removed it, thanks! http://gerrit.cloudera.org:8080/#/c/24578/15//COMMIT_MSG@32 PS15, Line 32: SetDiffScan overload takes a : 'DiffScanRowVisibil > nit: seems to be a duplication? Done http://gerrit.cloudera.org:8080/#/c/24578/15//COMMIT_MSG@56 PS15, Line 56: : Change-Id: I0d0727fb2b89c727c669ec218e3bb8594336359a : : > nit: as of PS15, the only SetDiffScan() left has three parameters (or maybe Rephrased it for clarity. 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: : // Insert rows > nit: create an utility function out of this and use here and elsewhere (at Done 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: > Per line 536 of PS15, kKey2 is 2, not -2, isn't it? It is stored as -2 as per below tablet-test-base.h (line 191): void IntKeyTestSetup<INT64>::BuildRowKey(KuduPartialRow *row, int64_t i) { CHECK_OK(row->SetInt64(0, (int64_t) i * (i % 2 == 0 ? -1 : 1))); } Added a comment above explaining this 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 Done http://gerrit.cloudera.org:8080/#/c/24578/15/src/kudu/tablet/diff_scan-test.cc@567 PS15, Line 567: > Any particular reason behind using EXPECT_xxx instead of ASSERT_xxx here an Nope, changed it to ASEERT_xxx http://gerrit.cloudera.org:8080/#/c/24578/15/src/kudu/tablet/diff_scan-test.cc@577 PS15, Line 577: > Per line 536 of PS15, kKey2 is 2, not -2, isn't it? Same as the above comment. Added an explanatory comment as well http://gerrit.cloudera.org:8080/#/c/24578/15/src/kudu/tablet/diff_scan-test.cc@598 PS15, Line 598: : : : : : : : : : : : : : : : : : : : : : : : : : : > Any chance of unifying this and OrderedDiffScanWithDeletesTest::DoScan()? Done -- 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: 9 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 19:01:38 +0000 Gerrit-HasComments: Yes
