Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18835 )
Change subject: KUDU-3353 [schema] Add an immutable attribute on column schema (part 2) ...................................................................... Patch Set 5: (16 comments) http://gerrit.cloudera.org:8080/#/c/18835/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18835/4//COMMIT_MSG@10 PS4, Line 10: This patch includes the C++ client-side changes of the "new column : attribute IMMUTABLE" feature, including: > This patch includes the C++ client-side changes of the "new column attribut Done http://gerrit.cloudera.org:8080/#/c/18835/4//COMMIT_MSG@12 PS4, Line 12: A new KuduColumnSpec::Immutable() method to add the attribute : to a column. > A new KuduColumnSpec::Immutable() method to add the attribute to a column. Done http://gerrit.cloudera.org:8080/#/c/18835/4//COMMIT_MSG@14 PS4, Line 14: A new KuduColumnSchema::is_immutable() to check if the attribute : is set for a column schema. > A new KuduColumnSchema::is_immutable() to check if the attribute is set for Done http://gerrit.cloudera.org:8080/#/c/18835/4//COMMIT_MSG@16 PS4, Line 16: A new KuduUpsertIgnore operation in the client API: use the : newly added KuduTable::NewUpsertIgnore() to create a new instance : of such operation. > A new KuduUpsertIgnore operation in the client API: use the newly added Kud Done http://gerrit.cloudera.org:8080/#/c/18835/4//COMMIT_MSG@19 PS4, Line 19: Unit tests to cover > Unit tests to cover the newly introduced functionality. Done http://gerrit.cloudera.org:8080/#/c/18835/4//COMMIT_MSG@20 PS4, Line 20: Small refactoring on the server side > Small refactoring on the server side. Done http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: PS4: > Could you please add a test to verify that the client gets proper error ret 1. Good idea! Tests of creating and altering table with immutable attribute column have been added. 2. It seems that xxx_IGNORE operations feature consult with server is only supported in Java/Scala client, C++ client lack of that. I'm not sure if it is reasonable to do that, since once the table with immutable attribute column has been created / altered, the cluster must supports this feature, the following UPSERT_IGNORE ops will be OK then. Of course, clients can be used by different apps, some app is used for creating table, and some for writing/reading table, the later will occur such issue then. http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/client-test.cc@243 PS4, Line 243: ->Default(KuduValue::FromInt(12345)); > Instead, is it possible to create a separate class that has its own schema Good idea! Made necessary changes and done. http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/client-test.cc@3064 PS4, Line 3064: > attempting Done http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/client-test.cc@3064 PS4, Line 3064: > having at least one column with the immutable attribute set Done http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/client-test.cc@3138 PS4, Line 3138: > drop Done http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/table_alterer-internal.cc File src/kudu/client/table_alterer-internal.cc: http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/table_alterer-internal.cc@131 PS4, Line 131: s.spec->data_->immutable > A couple of questions here: This is the delta schema of an altering operation, here is limit to alter the immutable attribute, that is to say, not allowed to change any column from immutable to mutable, and vice versa. Alter ops like changing name or compression is allowed for immutable columns, nothing will be broken. But you remind me to consider if it is reasonable for this limit, nothing will be broken if removing this limit. I've removed it. http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/write_op.h File src/kudu/client/write_op.h: http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/write_op.h@219 PS4, Line 219: errors on updating immutable : /// cells are ignored. > errors on updating immutable cells are ignored. Done http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/write_op.h@272 PS4, Line 272: and : /// errors on updating immutable cells are ignored. > ... and errors on updating immutable cells are ignored. Done http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/tablet/tablet.cc@a746 PS4, Line 746: > If so, does it make sense to just return the pre-b6eedb224 version of the c It's reasonable, done. http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/tablet/tablet.cc@a824 PS4, Line 824: : : : : : : : : : : > Ditto? Done -- To view, visit http://gerrit.cloudera.org:8080/18835 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id301e313eedd9cc3a494d6b4f3c14fbabe63b436 Gerrit-Change-Number: 18835 Gerrit-PatchSet: 5 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Wed, 24 Aug 2022 17:01:24 +0000 Gerrit-HasComments: Yes
