Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column Upsert support ...................................................................... Patch Set 2: (13 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 the value for an auto-incrementing column is allowed/disallowed. Also, providing a reasoning for the decision to allow/disallow specifying a value for the auto-incrementing column would be great, or provide a link to the design document instead where the such reasoning is present. http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG@7 PS2, Line 7: Upsert UPSERT http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG@9 PS2, Line 9: Upsert UPSERT http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG@9 PS2, Line 9: server side the server side 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 attributed to regular and designed (i.e. not exceptional) workflow. 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/editor?) 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 something). 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? http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@502 PS2, Line 502: } else { Make this first element of the corresponding 'if ()' clause, and since there is 'return' from this scope, it will be more readable then when removing the if/else scope for then the second part. http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@504 PS2, Line 504: err_field_incorrectly_set Correct this to be style-compliant. http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@511 PS2, Line 511: // 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())) { : op->SetFailureStatusOnce(row_status); : } Make this first part of the corresponding if/else for better readability since this one is short, and the other is long. http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@796 PS2, Line 796: /* if (tablet_schema_->has_auto_incrementing()) { : return Status::NotSupported( : Substitute("tables with auto-incrementing column do not support " : "$0 operations", RowOperationsPB_Type_Name(type))); : If this code isn't needed, just remove it, don't comment out (especially with such style of commenting). 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: * specify a value of the auto-incremeting column for operations that don't support that * specify an invalid value (e.g., a negative one) for the auto-incrementing column for operations that support setting the value * specify the maximum allowable value for the auto-incrementing column with UPSERT, and then try to insert one more row without specifying the value for the column -- 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: 2 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: Fri, 16 Jun 2023 18:29:32 +0000 Gerrit-HasComments: Yes
