Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/24578 )
Change subject: Add diff-scan option to return unobservable rows ...................................................................... Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/24578/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24578/7//COMMIT_MSG@14 PS7, Line 14: (INSERT -> eventual DELETE) > question: row-level history is not a requirement here - right ? Yes, history is not a requirement here. The latest state needs to be sent back and by default we do not send back if a row has been inserted and deleted in the scan snapshot window. This patch enables that behavior. http://gerrit.cloudera.org:8080/#/c/24578/7//COMMIT_MSG@40 PS7, Line 40: The RPC field is a new 'optional bool' and is wire-compatible with : older servers (which ignore it) and older clients (which don't set it). > How is it compatible with older servers when they simply cannot interpret t The needed feature flag (TabletServerFeatures::DIFF_SCAN_ROW_VISIBILITY) has been added. http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/client.cc@1912 PS7, Line 1912: Status KuduScanner::SetDiffScan(uint64_t start_timestamp, uint64_t end_timestamp) { : if (data_->open_) { : return Status::IllegalState("Diff scan must be set before Open()"); : } : return data_->mutable_configuration()->SetDiffScan(start_timestamp, end_timestamp); : } > nit: Could this just call the 3 param version with default visibility? Good call, done http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/client.proto File src/kudu/client/client.proto: http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/client.proto@101 PS7, Line 101: Unknown = 0; > Should the newly added field should come with a new feature flag? In other Yes, the needed feature flag has been added here as well. A legacy client receiving a token with the new flag will fail with Status::NotSupported(".."). http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/client.proto@169 PS7, Line 169: bool include_unobservable_rows = 26 > Consider introducing an enumeration here, similar to OrderMode and/or ReadM Done http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/scan_configuration.h File src/kudu/client/scan_configuration.h: http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/scan_configuration.h@94 PS7, Line 94: bool include_unobservable_rows = false > Any particular reason of not using DiffScanRowVisibility here as well? Not really, updated this to use the enum parameter. http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/scan_configuration.h@230 PS7, Line 230: bool include_unobservable_rows_; > Would it make more sense to store KuduScanner::DiffScanRowVisibility here i Done http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/tablet/rowset.h@116 PS7, Line 116: include_unobservable_rows > Would it make sense to store KuduScanner::DiffScanRowVisibility here insted Done http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/tserver/tserver.proto@361 PS7, Line 361: // When set on a diff scan, rows whose entire lifecycle lies inside : // (snap_start_timestamp, snap_timestamp] i.e. rows that were inserted : // and deleted within the range will still be returned (marked deleted via : // the IS_DELETED virtual column). : // : // Only valid on diff scans. The server returns INVALID_SCAN_SPEC if this : // field is set without snap_start_timestamp, or on a diff scan whose : // projection lacks an IS_DELETED virtual column. : optional bool include_unobservable_rows = 17; > Shouldn't this update come along with a new feature flag added into TabletS TabletServerFeatures::DIFF_SCAN_ROW_VISIBILITY is added -- 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: 7 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, 14 Aug 2026 23:30:02 +0000 Gerrit-HasComments: Yes
