Alexey Serbin 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 4: (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 feature : "new column attribute IMMUTABLE", includes: This patch includes the C++ client-side changes of the "new column attribute IMMUTABLE" feature, including: http://gerrit.cloudera.org:8080/#/c/18835/4//COMMIT_MSG@12 PS4, Line 12: Add a new API 'KuduColumnSpec.Immutable()' to a column schema : to be immutable. A new KuduColumnSpec::Immutable() method to add the attribute to a column. http://gerrit.cloudera.org:8080/#/c/18835/4//COMMIT_MSG@14 PS4, Line 14: Add a new API 'KuduColumnSchema.is_immutable()' to judge if : a column schema has the immutable attribute set. A new KuduColumnSchema::is_immutable() to check if the attribute is set for a column schema. http://gerrit.cloudera.org:8080/#/c/18835/4//COMMIT_MSG@16 PS4, Line 16: Add a new operation 'KuduUpsertIgnore' in client interface, : user can use the new add function 'KuduTable.NewUpsertIgnore()' : to new such an operation. A new KuduUpsertIgnore operation in the client API: use the newly added KuduTable::NewUpsertIgnore() to create a new instance of such operation. http://gerrit.cloudera.org:8080/#/c/18835/4//COMMIT_MSG@19 PS4, Line 19: Related unit tests. Unit tests to cover the newly introduced functionality. http://gerrit.cloudera.org:8080/#/c/18835/4//COMMIT_MSG@20 PS4, Line 20: Some small refactors in server side. Small refactoring on the server side. 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 returned from a server that doesn't support the newly introduced column attribute while trying to: * create a table with a column having immutable attribute * alter a table, adding a column with the immutable attribute BTW, what happens if a new client tries to send UPSERT_IGNORE operation to a server that doesn't support that feature? http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/client-test.cc@243 PS4, Line 243: b.AddColumn("imm_val")->Type(KuduColumnSchema::INT32)->Immutable()->Nullable(); Instead, is it possible to create a separate class that has its own schema with a column with the immutable attribute set (it might be derived from ClientTest)? Changing this generic schema right here brings in too many changes in not quite related scenarios. http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/client-test.cc@3064 PS4, Line 3064: with immutable column set having at least one column with the immutable attribute set http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/client-test.cc@3064 PS4, Line 3064: attempt attempting http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/client-test.cc@3138 PS4, Line 3138: set drop 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: (a) Why to limit altering immutable columns if looking at this from the conceptual point of view? Are there any constraints that would be broken if allowing, say, changing the name or compression for an immutable column? (b) Do we have the corresponding check at the server-side for this? If changing some attributes of an immutable column (such as name, encoding, etc.) breaks server-side invariants, the enforcement should be at the server side as well. Adding that into the client is just a nice-to-have thing to reduce number of client-server roundtrips, but the server-side invariants should be enforced by the server side. 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: updating on a row with : /// immutable cells errors are ignored. errors on updating immutable cells are ignored. http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/client/write_op.h@272 PS4, Line 272: or : /// updating on a row with immutable cells errors are ignored. ... and errors on updating immutable cells are ignored. 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: > ApplyUpsertAsUpdate will not return Immutable status when op is UPSERT_IGNO If so, does it make sense to just return the pre-b6eedb224 version of the code here, i.e.: return ApplyUpsertAsUpdate(io_context, op_state, op, op->present_in_rowset, stats); ? http://gerrit.cloudera.org:8080/#/c/18835/4/src/kudu/tablet/tablet.cc@a824 PS4, Line 824: : : : : : : : : : : Ditto? -- 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: 4 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: Tue, 23 Aug 2022 01:47:02 +0000 Gerrit-HasComments: Yes
