Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18241 )
Change subject: KUDU-3353 [schema] Add an immutable attribute on column schema ...................................................................... Patch Set 19: (44 comments) Thank you for the patch! I started reviewing this patch, accumulated some review feedback and then found that too many updates are incorporated into a single patch. I think that for tracking the changes in posterity and for the review process it would be great to post the following patches separately, in this dependency/base order: * non-essential updates on the original code * changes on proto files, the server side, and C++ client (including the C++ client part just to have enough test coverage for the former two, but it might be separated as well) * corresponding updates on the Java client * corresponding updates on the Scala bindings http://gerrit.cloudera.org:8080/#/c/18241/18//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18241/18//COMMIT_MSG@10 PS18, Line 10: been written nit: it's been written during inserting the row http://gerrit.cloudera.org:8080/#/c/18241/19//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18241/19//COMMIT_MSG@7 PS19, Line 7: immutable attribute on column schema It would be great to add a blurb to describe the use case that requires the immutable attribute on a column to be present. I guess describing that either in the description of this commit and/or in the KUDU-3353 JIRA item is fine. http://gerrit.cloudera.org:8080/#/c/18241/19//COMMIT_MSG@13 PS19, Line 13: 3. Since the column is immutable, we restrict it must be 'NOT NULL'. : Otherwise, you can't update the NULL value after the initial insertion. I'm not sure I understand the reasoning. The same might be applied to any other value, no? Why NULL is so special in this case? http://gerrit.cloudera.org:8080/#/c/18241/19//COMMIT_MSG@15 PS19, Line 15: able possible http://gerrit.cloudera.org:8080/#/c/18241/19//COMMIT_MSG@15 PS19, Line 15: All the old column : data in the table has the default immutable value, new insertion can : specify a cell value on the column or not, if not, default value will be : used. Is this any different from the behavior of any other column that has default value? If not, why to mention this? http://gerrit.cloudera.org:8080/#/c/18241/19/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java: PS19: Could you please separate the update of the Java client into a separate patch? http://gerrit.cloudera.org:8080/#/c/18241/19/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java: PS19: Could you please separate the update of the Java client into a separate patch? http://gerrit.cloudera.org:8080/#/c/18241/19/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java@195 PS19, Line 195: ignore updating on immutable columns ignore errors when trying to update immutable columns http://gerrit.cloudera.org:8080/#/c/18241/19/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java File java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java: PS19: Could you please separate the update of the Java client into a separate patch? http://gerrit.cloudera.org:8080/#/c/18241/19/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java: PS19: Could you please separate the update of the Java client into a separate patch? http://gerrit.cloudera.org:8080/#/c/18241/19/java/kudu-client/src/main/java/org/apache/kudu/client/Status.java File java/kudu-client/src/main/java/org/apache/kudu/client/Status.java: PS19: Could you please separate the update of the Java client into a separate patch? http://gerrit.cloudera.org:8080/#/c/18241/19/java/kudu-client/src/main/java/org/apache/kudu/client/UpsertIgnore.java File java/kudu-client/src/main/java/org/apache/kudu/client/UpsertIgnore.java: PS19: Could you please separate the update of the Java client into a separate patch? http://gerrit.cloudera.org:8080/#/c/18241/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java: PS19: Could you please separate the update of the Java client into a separate patch? http://gerrit.cloudera.org:8080/#/c/18241/19/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala: PS19: Could you please move the update on the Scala bindings into a separate patch? http://gerrit.cloudera.org:8080/#/c/18241/19/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala: PS19: Could you move the update on Scala bindings into a separate patch? http://gerrit.cloudera.org:8080/#/c/18241/19/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/OperationType.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/OperationType.scala: http://gerrit.cloudera.org:8080/#/c/18241/19/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/OperationType.scala@56 PS19, Line 56: private[kudu] case object UpsertIgnore extends OperationType { : override def operation(table: KuduTable): Operation = table.newUpsertIgnore() : : override def toString(): String = "upsert_ignore" : } Could you move the updates on Scala bindings into a separate patch? http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/client/write_op.h File src/kudu/client/write_op.h: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/client/write_op.h@144 PS19, Line 144: ~KuduInsert() override; Here and below: please move this and other non-essential update into a separate patch (OVERRIDE --> override, etc.) http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/common/row.h File src/kudu/common/row.h: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/common/row.h@254 PS19, Line 254: // projection_ column index -> base_schema_ index Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/common/row_changelist.h File src/kudu/common/row_changelist.h: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/common/row_changelist.h@254 PS19, Line 254: for (int i = schema->num_key_columns(); i < schema->num_columns(); ++i) { Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/common/row_changelist.cc File src/kudu/common/row_changelist.cc: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/common/row_changelist.cc@a266 PS19, Line 266: Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/common/row_changelist.cc@282 PS19, Line 282: const void* new_val = nullptr; Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/common/schema-test.cc File src/kudu/common/schema-test.cc: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/common/schema-test.cc@236 PS19, Line 236: nullptr, nullptr Please move this non-essential update (NULL --> nullptr) into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/18/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/18241/18/src/kudu/common/schema.h@208 PS18, Line 208: TODO(yingchun): update comments Indeed -- it would be great to document the 'is_immutable' parameter http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/delta_applier.h File src/kudu/tablet/delta_applier.h: PS19: Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/delta_applier.cc File src/kudu/tablet/delta_applier.cc: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/delta_applier.cc@a46 PS19, Line 46: Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/delta_iterator_merger.h File src/kudu/tablet/delta_iterator_merger.h: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/delta_iterator_merger.h@84 PS19, Line 84: bool HasNext() const override; Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/delta_iterator_merger.cc File src/kudu/tablet/delta_iterator_merger.cc: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/delta_iterator_merger.cc@122 PS19, Line 122: bool DeltaIteratorMerger::HasNext() const Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/delta_iterator_merger.cc@144 PS19, Line 144: ret.append(JoinMapped(iters_, [](const unique_ptr<DeltaIterator> &iter) { : return iter->ToString(); : }, ", ")); Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/delta_store.h@343 PS19, Line 343: DELETEs Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/delta_store.h@365 PS19, Line 365: virtual bool HasNext() const = 0; Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/delta_store.h@561 PS19, Line 561: deleted_, Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/deltafile.h File src/kudu/tablet/deltafile.h: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/deltafile.h@270 PS19, Line 270: bool HasNext() const override; Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/deltafile.cc File src/kudu/tablet/deltafile.cc: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/deltafile.cc@736 PS19, Line 736: bool DeltaFileIterator<Type>::HasNext() const { Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/deltamemstore-test.cc File src/kudu/tablet/deltamemstore-test.cc: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/deltamemstore-test.cc@a639 PS19, Line 639: Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/deltamemstore-test.cc@600 PS19, Line 600: Status s = dms_->NewDeltaIterator(opts, &iter); Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/deltamemstore.h File src/kudu/tablet/deltamemstore.h: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/deltamemstore.h@242 PS19, Line 242: bool HasNext() const override; Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/deltamemstore.cc File src/kudu/tablet/deltamemstore.cc: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/deltamemstore.cc@305 PS19, Line 305: bool DMSIterator::HasNext() const { Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/ops/op.h File src/kudu/tablet/ops/op.h: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/ops/op.h@31 PS19, Line 31: #include "kudu/common/schema.h" Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/rowset.h@43 PS19, Line 43: class Arena; Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/rowset.cc File src/kudu/tablet/rowset.cc: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/rowset.cc@a146 PS19, Line 146: Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/tablet-decoder-eval-test.cc File src/kudu/tablet/tablet-decoder-eval-test.cc: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/tablet-decoder-eval-test.cc@78 PS19, Line 78: ColumnSchema("string_val_a", STRING, true, false, : nullptr, nullptr, : ColumnStorageAttributes(DICT_ENCODING, : DEFAULT_COMPRESSION)), : ColumnSchema("string_val_b", STRING, true, false, : nullptr, nullptr, : ColumnStorageAttributes(DICT_ENCODING, : DEFAULT_COMPRESSION))}, 1)) Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/tablet.cc@705 PS19, Line 705: if (PREDICT_FALSE(rcl_decoder.is_reinsert())) { Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/tablet.cc@763 PS19, Line 763: DCHECK(!op->present_in_rowset); Please move this non-essential update into a separate patch. http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/tablet_metrics.cc File src/kudu/tablet/tablet_metrics.cc: http://gerrit.cloudera.org:8080/#/c/18241/19/src/kudu/tablet/tablet_metrics.cc@46 PS19, Line 46: due to an error If there is any other error except for the immutability of a column in this context? If not, then maybe mention the only error that's counted by this metric. -- To view, visit http://gerrit.cloudera.org:8080/18241 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If80ebca7d3ab14db1591c14201f6d561155526cd Gerrit-Change-Number: 18241 Gerrit-PatchSet: 19 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Thu, 07 Jul 2022 02:16:38 +0000 Gerrit-HasComments: Yes
