Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/21695 )
Change subject: KUDU-3606 [tools] Check column IDs when --strict_column_id is enabled ...................................................................... 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. Done http://gerrit.cloudera.org:8080/#/c/21695/4//COMMIT_MSG@9 PS4, Line 9: ensure > ensures Done http://gerrit.cloudera.org:8080/#/c/21695/4//COMMIT_MSG@10 PS4, Line 10: user faced > user-facing Done http://gerrit.cloudera.org:8080/#/c/21695/4//COMMIT_MSG@11 PS4, Line 11: to > as Done 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? Done 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 se It has multiple 'hole' intervals now, they are: key int_val1 string_val1 <hole1: deleted to_delete_col1> int_val2 <hole2: deleted to_delete_col2, to_delete_col3> string_val2 <hole3: deleted to_delete_col4> 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 ... Done 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. Done 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 someth Done -- 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 <laiyingc...@apache.org> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com> Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Comment-Date: Fri, 13 Sep 2024 08:40:20 +0000 Gerrit-HasComments: Yes
