Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column Upsert support ...................................................................... Patch Set 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG Commit Message: PS2: > It would be great to outline the matrix of the operations where specifying Done http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG@7 PS2, Line 7: Upsert > UPSERT Done http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG@9 PS2, Line 9: server side > the server side Done http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG@9 PS2, Line 9: Upsert > UPSERT Done http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@477 PS2, Line 477: PREDICT_FALSE > It's not a good idea to use PREDICT_FALSE for conditions that are attribute Done http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@484 PS2, Line 484: uint8_t * > correct this to be style-compliant (maybe, it's time to configure your IDE/ Done http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@490 PS2, Line 490: err_value > https://google.github.io/styleguide/cppguide.html#Constant_Names Done, thanks http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@490 PS2, Line 490: IllegalState > Should this be InvalidArgument if it's coming from the write request? Done http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@490 PS2, Line 490: err_value > Correct this to be style-compliant (should be in form cErrValue or somethin Done http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@502 PS2, Line 502: } > Make this first element of the corresponding 'if ()' clause, and since ther Done http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@504 PS2, Line 504: column values not to be > Correct this to be style-compliant. Done http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@511 PS2, Line 511: } else { : // Copy the non-null value. : Status row_status; : RETURN_NOT_OK(ReadColumn( : col, tablet_row.mutable_cell_ptr(tablet_col_idx), &row_status)); : if (PREDICT_FALSE(!row_status.ok())) { : > Make this first part of the corresponding if/else for better readability si Done http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@796 PS2, Line 796: case RowOperationsPB::UPSERT_IGNORE: : /* if (tablet_schema_->has_auto_incrementing()) { : return Status::NotSupported( : Substitute("tables with auto-incrementing column do not support " : > If this code isn't needed, just remove it, don't comment out (especially wi Ah, this was meant to be removed. Thanks for catching it. http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/tablet/tablet_auto_incrementing-test.cc File src/kudu/tablet/tablet_auto_incrementing-test.cc: http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/tablet/tablet_auto_incrementing-test.cc@161 PS2, Line 161: } > It would be great to add some coverage for the following conditions: Done -- To view, visit http://gerrit.cloudera.org:8080/20061 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47 Gerrit-Change-Number: 20061 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Wed, 21 Jun 2023 21:39:45 +0000 Gerrit-HasComments: Yes
