Ashwani Raina 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: (3 comments) http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/client/client.h@3154 PS9, Line 3154: DiffScanRowVisibility nit: Should this have 1-to-1 mapping with RowVisibility with addition of default UNKNOWN_ROW_VISIBILITY? http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/client/scan_token-internal.cc@301 PS9, Line 301: const auto visibility = message.row_visibility() == kudu::INCLUDE_UNOBSERVABLE : ? KuduScanner::INCLUDE_UNOBSERVABLE : : KuduScanner::OBSERVABLE_ONLY; q: Is 'visibility' intentionally set to OBSERVABLE_ONLY if message's row_visibility is UNKNOWN_ROW_VISIBILITY for any reason? Or such case needs to be reported or handled? http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/client/scan_token-internal.cc@458 PS9, Line 458: DCHECK(configuration_.has_start_timestamp() && : configuration_.has_snapshot_timestamp()) : << "row_visibility=INCLUDE_UNOBSERVABLE requires a diff scan " : "(both start and snapshot timestamps must be set)"; Should this be a CHECK instead, to align with similar CHECK inside ScanConfiguration::AddIsDeletedColumn() for has_start_timestamp() and has_snapshot_timestamp()? Also, having a CHECK instead of DCHECK could catch any inadvertent/independent setting of row_visibility_, that would break the tight relation between the visibility and start/snapshot timestamps. -- 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: Tue, 18 Aug 2026 09:52:09 +0000 Gerrit-HasComments: Yes
