Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19097 )
Change subject: KUDU-1945 Auto-Incrementing Column ...................................................................... Patch Set 10: (15 comments) http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/client/client-test.cc@9585 PS8, Line 9585: s.ToString(), > set auto_incrementing column as primary key We cannot as that was not yet implemented in this patch. In the followup patch by Marton is where that is implemented. http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/client/client-test.cc@9618 PS8, Line 9618: > What about UPSERT operation? How the auto-incrementing column should be sp As discussed UPSERT operation will be not supported in this initial version. I'll add the needed checks. http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/client/client-test.cc@9638 PS8, Line 9638: .Create()); > add some negative test cases, like set value for auto_incrementing_id colum Done http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/client/client-test.cc@9638 PS8, Line 9638: .Create()); > +1 Done http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/client/schema.cc@488 PS8, Line 488: //if (!data_->primary_key) { > auto_incrementing column should be primary_key, consolidate all sanity chec Done http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/common/row_operations.cc@498 PS8, Line 498: "value reached"); > style nit: create a constexpr for this message and use in both Status objec Done http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/common/row_operations.cc@500 PS8, Line 500: return max_value; > Since there might be many columns encoded in a write operation PB, the coun We can increment the counter regardless of future failures as the key space is large enough to not run out of values. I'll add a comment. http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/common/schema.h@221 PS8, Line 221: write_default: default value added to the row if the column value was : // not specified on insert. : // The value will be copied and released on ColumnSchema destruction. : // comment: the comment for the column. > move this block to line #215 Thanks, good catch http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/common/schema.h@231 PS8, Line 231: ColumnSchema col_c("c", INT32, false, false, false, &default_i32); : // Slice default_str("Hello"); : // ColumnSchema col_d("d", STRING, false, false, false, &default_str); : // ColumnSchema col_e("e", STRING, false, false, true, &default_str); > is_auto_incrementing should be added as 5-th parameter Thanks, good catch http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/common/schema.cc@653 PS8, Line 653: e read/write default > nit: copy/paste error Done http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tablet/tablet_auto_incrementing-test.cc File src/kudu/tablet/tablet_auto_incrementing-test.cc: http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tablet/tablet_auto_incrementing-test.cc@80 PS8, Line 80: > style nit: add spaces around '+' Done http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tablet/tablet_auto_incrementing-test.cc@81 PS8, Line 81: } > add some negative test cases here. Done http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tablet/tablet_replica.h@44 PS8, Line 44: #include "kudu/tablet/tablet.h" // IWYU pragma: keep > Why to keep this header file added and also keeping the forward declaration Thanks for pointing that out. I think that was due to LINT pointed issues previously. Fixed it now http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tserver/tserver.proto@140 PS8, Line 140: // Value of the auto-incrementing counter > nit: remove this empty extra line? Done http://gerrit.cloudera.org:8080/#/c/19097/8/src/kudu/tserver/tserver.proto@144 PS8, Line 144: // A batched set of insert/mutate requests. > nit: add an empty line to separate different messages Done -- To view, visit http://gerrit.cloudera.org:8080/19097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dbde9095da78f6d1bd00adcc0a6e7dd63082bbc Gerrit-Change-Number: 19097 Gerrit-PatchSet: 10 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: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Mon, 19 Dec 2022 08:14:04 +0000 Gerrit-HasComments: Yes
