Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24158 )
Change subject: [tablet] optimize MemRowSet scan to single-pass changelist application ...................................................................... Patch Set 1: Looks good, can you please add some more tests described below? The new test only scans with CreateSnapshotIncludingAllOps(). Could you add a case that saves a snapshot between two mutations (e.g. UPDATE col_a at T1, UPDATE col_b at T2) and then scans at T1 with a projection that excludes col_a? The snap_to_exclude (closed time-range) code path is not covered by the new test at all. TestScanSnapToExclude exercises that path exhaustively but uses the 2-column schema so the kColumnNotFound skip branch is never reached there either. Could you add a case using the wide schema with a [snap_exclude, snap_include) window scan and a partial projection that omits some of the mutated columns? The current test only inserts a single row. Could you add a few more rows (with different mutation chains) so the iterator's outer row loop is exercised with column-skipping active? No test covers a key-only projection (no value columns). With the new code the while (decoder.HasNext()) loop runs but every entry returns kColumnNotFound and nothing is applied. Worth a single smoke case asserting the scan completes cleanly and returns the correct key values. Include_deleted_rows combined with a partial projection is not tested. TestScanIncludeDeletedRows covers deleted-row visibility but always uses a full-schema projection. Consider adding a case with include_deleted_rows=true on the wide schema where some value columns are omitted, to cover the interaction between the deleted-row selection logic and the new column-skip path. -- To view, visit http://gerrit.cloudera.org:8080/24158 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ccdfcf207680261907d025b64a9b046bb108f13 Gerrit-Change-Number: 24158 Gerrit-PatchSet: 1 Gerrit-Owner: Yan-Daojiang <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Yan-Daojiang <[email protected]> Gerrit-Comment-Date: Wed, 08 Apr 2026 11:33:15 +0000 Gerrit-HasComments: No
