Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21695 )
Change subject: KUDU-3606 [tools] Copy table with non-continuous column ids ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/21695/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21695/4//COMMIT_MSG@7 PS4, Line 7: Copy table with non-continuous column ids This doesn't seem to match with the rest of the description for this patch. Is this up-to-date? http://gerrit.cloudera.org:8080/#/c/21695/4//COMMIT_MSG@9 PS4, Line 9: ensure ensures http://gerrit.cloudera.org:8080/#/c/21695/4//COMMIT_MSG@10 PS4, Line 10: user faced user-facing http://gerrit.cloudera.org:8080/#/c/21695/4//COMMIT_MSG@11 PS4, Line 11: to as http://gerrit.cloudera.org:8080/#/c/21695/4//COMMIT_MSG@19 PS4, Line 19: A new flag --show_column_id is also added to : indicate whether to show column ids when using : 'kudu table describe'. Could you please move this part into a separate patch? http://gerrit.cloudera.org:8080/#/c/21695/4/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/21695/4/src/kudu/tools/kudu-tool-test.cc@1281 PS4, Line 1281: alterer->DropColumn("to_delete_col1") : ->DropColumn("to_delete_col2") : ->DropColumn("to_delete_col3") : ->DropColumn("to_delete_col4"); This creates just a single 'hole' in the column ID space. Would it make sense to test multiple 'hole' intervals as well? http://gerrit.cloudera.org:8080/#/c/21695/4/src/kudu/tools/table_scanner.cc File src/kudu/tools/table_scanner.cc: http://gerrit.cloudera.org:8080/#/c/21695/4/src/kudu/tools/table_scanner.cc@147 PS4, Line 147: to consider the column id ... to compare column IDs ... http://gerrit.cloudera.org:8080/#/c/21695/4/src/kudu/tools/table_scanner.cc@147 PS4, Line 147: The destination table can " : "have different column ids, for example, the destination table is not created by the " : "latest 'kudu table copy' tool, but we can still use the tool to copy data by files " : "from different table though 'kudu remote_replica copy'. Maybe, try to rephrase this a bit to make it easier to read and comprehend. Imagine this is read by somebody who doesn't have a lot of context on table copying. Instead of describing the case that works without strict schema comparison, describe the case when it matters, so it's easier to comprehend why it might make sense to customize the setting for this flag. http://gerrit.cloudera.org:8080/#/c/21695/4/src/kudu/tools/table_scanner.cc@493 PS4, Line 493: CHECK_LT(expect_column_id, actual_column_id); Instead of crashing in this case, maybe return Status::Corruption or something? I don't like the idea of crashing in non-debug builds when there is a way to recover from an error condition by just returning an error to the upper level. -- To view, visit http://gerrit.cloudera.org:8080/21695 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77af7b92b45f3866cc8b699e61b9e71b73ed6c4b Gerrit-Change-Number: 21695 Gerrit-PatchSet: 4 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 27 Aug 2024 22:19:03 +0000 Gerrit-HasComments: Yes
