Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24845 )
Change subject: [tserver] Validate deprecated range predicate column type. ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/24845/1//COMMIT_MSG Commit Message: PS1: Please add a test to verify that the new code responds with an error as expected -- that at least should help catch regressions in the future. http://gerrit.cloudera.org:8080/#/c/24845/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/24845/1/src/kudu/tserver/tablet_service.cc@2790 PS1, Line 2790: int32_t idx nit: add 'const' to be explicit 'idx' is not going to change? const int32_t idx = ... http://gerrit.cloudera.org:8080/#/c/24845/1/src/kudu/tserver/tablet_service.cc@2791 PS1, Line 2791: (idx == Schema::kColumnNotFound) nit: does it make sense to wrap this into PREDICT_FALSE()? http://gerrit.cloudera.org:8080/#/c/24845/1/src/kudu/tserver/tablet_service.cc@2792 PS1, Line 2792: return Status::InvalidArgument( : string("Invalid predicate ") + SecureShortDebugString(pred_pb) + : ": unknown column."); nit for here and below: switch to Substitute() (strings::Substitute()) for formatting the error messages http://gerrit.cloudera.org:8080/#/c/24845/1/src/kudu/tserver/tablet_service.cc@2799 PS1, Line 2799: nit: this seems to be wrong indent -- should be +2 more spaces for expr continuation on next string -- To view, visit http://gerrit.cloudera.org:8080/24845 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82ad53ad2263ca587e81fd37d64cc7982060b0f1 Gerrit-Change-Number: 24845 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Martonka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 15 Sep 2026 16:46:28 +0000 Gerrit-HasComments: Yes
