You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2024/08/20 15:19:44 UTC

[kudu-CR] Copy table with non-continuous column ids

Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21695


Change subject: Copy table with non-continuous column ids
......................................................................

Copy table with non-continuous column ids

Change-Id: I77af7b92b45f3866cc8b699e61b9e71b73ed6c4b
---
M src/kudu/client/schema.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/tool_action_table.cc
4 files changed, 168 insertions(+), 32 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/21695/1
-- 
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: newchange
Gerrit-Change-Id: I77af7b92b45f3866cc8b699e61b9e71b73ed6c4b
Gerrit-Change-Number: 21695
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <la...@apache.org>

[kudu-CR] [tools] Copy table with non-continuous column ids

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.

Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21695 )

Change subject: [tools] Copy table with non-continuous column ids
......................................................................


Patch Set 3:

Build Started http://jenkins.kudu.apache.org/job/pre_commit/443/


-- 
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: 3
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Wed, 21 Aug 2024 16:20:41 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3606 [tools] Check column IDs when --strict column id is enabled

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/21695 )

Change subject: KUDU-3606 [tools] Check column IDs when --strict_column_id is enabled
......................................................................

KUDU-3606 [tools] Check column IDs when --strict_column_id is enabled

This patch ensures the destination table has the
same user-facing schema and internal column ids
as the source table when using 'kudu table copy'.

If they are different, more details, including
column ids, default values, will be shown in
error messages. A new flag --strict_column_id is
added to indicate whether to compare column ids
when checking if the schemas match.

Change-Id: I77af7b92b45f3866cc8b699e61b9e71b73ed6c4b
Reviewed-on: http://gerrit.cloudera.org:8080/21695
Tested-by: Yingchun Lai <la...@apache.org>
Reviewed-by: Mahesh Reddy <mr...@cloudera.com>
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 183 insertions(+), 32 deletions(-)

Approvals:
  Yingchun Lai: Verified
  Mahesh Reddy: Looks good to me, but someone else must approve
  Alexey Serbin: Looks good to me, approved

-- 
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: merged
Gerrit-Change-Id: I77af7b92b45f3866cc8b699e61b9e71b73ed6c4b
Gerrit-Change-Number: 21695
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>

[kudu-CR] KUDU-3606 [tools] Check column IDs when --strict column id is enabled

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.

Kudu Jenkins 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 5:

Build Started http://jenkins.kudu.apache.org/job/pre_commit/508/


-- 
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: 5
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Fri, 13 Sep 2024 08:40:08 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3606 [tools] Check column IDs when --strict column id is enabled

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.

Kudu Jenkins 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 6: Verified-1

Build Failed 

http://jenkins.kudu.apache.org/job/pre_commit/511/ : FAILURE


-- 
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: 6
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Fri, 13 Sep 2024 17:05:47 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3606 [tools] Check column IDs when --strict column id is enabled

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.

Kudu Jenkins 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 7:

Build Started http://jenkins.kudu.apache.org/job/pre_commit/524/


-- 
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: 7
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Wed, 18 Sep 2024 09:17:15 +0000
Gerrit-HasComments: No

[kudu-CR] [tools] Copy table with non-continuous column ids

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/21695 )

Change subject: [tools] Copy table with non-continuous column ids
......................................................................


Patch Set 3:

The failed tests (org.apache.kudu.client.TestSecurity, client-test.4, org.apache.kudu.client.TestKuduClient) are not related.


-- 
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: 3
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Thu, 22 Aug 2024 02:28:29 +0000
Gerrit-HasComments: No

[kudu-CR] [tools] Copy table with non-continuous column ids

Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.

Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21695 )

Change subject: [tools] Copy table with non-continuous column ids
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/21695/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21695/3//COMMIT_MSG@12
PS3, Line 12: it
            : will report an error saying that they have different
            : schemas, but the difference is not shown.
Before this patch we compared schemas of tables when doing table copy, seems we did not compare column ids IIUC. So what kind of error do we have if two tables' column ids are not same?


http://gerrit.cloudera.org:8080/#/c/21695/3//COMMIT_MSG@22
PS3, Line 22: A new flag --show_column_id is also added to
            : indicate whether to show column ids when using
            : 'kudu table describe'.
nit: This part of change should be put in another patch, because it is for the 'kudu table describe' tool instead of 'kudu table copy' tool.


http://gerrit.cloudera.org:8080/#/c/21695/3/src/kudu/tools/table_scanner.cc
File src/kudu/tools/table_scanner.cc:

http://gerrit.cloudera.org:8080/#/c/21695/3/src/kudu/tools/table_scanner.cc@446
PS3, Line 446: auto
nit: Is this change necessary?



-- 
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: 3
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Thu, 22 Aug 2024 08:48:40 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3606 [tools] Check column IDs when --strict column id is enabled

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Hello Mahesh Reddy, Alexey Serbin, Yifan Zhang, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21695

to look at the new patch set (#6).

Change subject: KUDU-3606 [tools] Check column IDs when --strict_column_id is enabled
......................................................................

KUDU-3606 [tools] Check column IDs when --strict_column_id is enabled

This patch ensures the destination table has the
same user-facing schema and internal column ids
as the source table when using 'kudu table copy'.

If they are different, more details, including
column ids, default values, will be shown in
error messages. A new flag --strict_column_id is
added to indicate whether to compare column ids
when checking if the schemas match.

Change-Id: I77af7b92b45f3866cc8b699e61b9e71b73ed6c4b
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 183 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/21695/6
-- 
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: newpatchset
Gerrit-Change-Id: I77af7b92b45f3866cc8b699e61b9e71b73ed6c4b
Gerrit-Change-Number: 21695
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>

[kudu-CR] KUDU-3606 [tools] Check column IDs when --strict column id is enabled

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin 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 6: Code-Review+1

(3 comments)

just a few nits, otherwise LGTM

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:     RETURN_NOT_OK(alterer->DropColumn("to_delete_col1")
              :         ->DropColumn("to_delete_col2")
              :         ->DropColumn("to_delete_col3")
              :         ->DropColumn("to_delete_col4")
> It has multiple 'hole' intervals now, they are:
Ah, indeed -- I missed that even if it's obvious from lines 1262 -- 1271


http://gerrit.cloudera.org:8080/#/c/21695/6/src/kudu/tools/table_scanner.cc
File src/kudu/tools/table_scanner.cc:

http://gerrit.cloudera.org:8080/#/c/21695/6/src/kudu/tools/table_scanner.cc@496
PS6, Line 496: must be in monotonic increase order
must be monotonically increasing


http://gerrit.cloudera.org:8080/#/c/21695/6/src/kudu/tools/table_scanner.cc@497
PS6, Line 497: expect
expecting



-- 
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: 6
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Tue, 17 Sep 2024 23:32:12 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] Copy table with non-continuous column ids

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/21695 )

Change subject: [tools] Copy table with non-continuous column ids
......................................................................


Patch Set 2:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/21695/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21695/2//COMMIT_MSG@11
PS2, Line 11: existed distination
> nit: existing destination
Done


http://gerrit.cloudera.org:8080/#/c/21695/2//COMMIT_MSG@19
PS2, Line 19: check
            : the consistent of shcemas.
> nit: checking if the schemas match.
Done


http://gerrit.cloudera.org:8080/#/c/21695/2/src/kudu/tools/table_scanner.cc
File src/kudu/tools/table_scanner.cc:

http://gerrit.cloudera.org:8080/#/c/21695/2/src/kudu/tools/table_scanner.cc@147
PS2, Line 147: compare
> nit: comparing
Done


http://gerrit.cloudera.org:8080/#/c/21695/2/src/kudu/tools/table_scanner.cc@147
PS2, Line 147: It's allowed the destination "
             :             "table to
> nit: The destination table can
Done


http://gerrit.cloudera.org:8080/#/c/21695/2/src/kudu/tools/table_scanner.cc@421
PS2, Line 421: CheckSchemaMatched
> nit: rename to 'SchemasMatch'?
Done


http://gerrit.cloudera.org:8080/#/c/21695/2/src/kudu/tools/table_scanner.cc@428
PS2, Line 428: using
> nit: uses
Done


http://gerrit.cloudera.org:8080/#/c/21695/2/src/kudu/tools/table_scanner.cc@463
PS2, Line 463: Destination
> nit: destination
Done


http://gerrit.cloudera.org:8080/#/c/21695/2/src/kudu/tools/table_scanner.cc@631
PS2, Line 631: the schemas are matched.
> nit: that the schemas match.
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: 2
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Wed, 21 Aug 2024 16:20:36 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3606 [tools] Check column IDs when --strict column id is enabled

Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.

Mahesh Reddy 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 7: Code-Review+1


-- 
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: 7
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Wed, 18 Sep 2024 21:25:09 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3606 [tools] Copy table with non-continuous column ids

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.

Kudu Jenkins 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: Verified-1

Build Failed 

http://jenkins.kudu.apache.org/job/pre_commit/447/ : FAILURE


-- 
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 <la...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Fri, 23 Aug 2024 08:38:41 +0000
Gerrit-HasComments: No

[kudu-CR] [tools] Copy table with non-continuous column ids

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/21695 )

Change subject: [tools] Copy table with non-continuous column ids
......................................................................


Patch Set 3: Code-Review+1


-- 
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: 3
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Thu, 22 Aug 2024 02:28:37 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3606 [tools] Check column IDs when --strict column id is enabled

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

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 7: Verified+1

The failed tests ts_recovery-itest, tablet_server-stress-test and client-test are not related.


-- 
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: 7
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Wed, 18 Sep 2024 15:59:00 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3606 [tools] Copy table with non-continuous column ids

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

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 <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Tue, 27 Aug 2024 22:19:03 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3606 [tools] Check column IDs when --strict column id is enabled

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

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 <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Fri, 13 Sep 2024 08:40:20 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] Copy table with non-continuous column ids

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/21695 )

Change subject: [tools] Copy table with non-continuous column ids
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/21695/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21695/3//COMMIT_MSG@12
PS3, Line 12: it
            : will report an error saying that they have different
            : schemas, but the difference is not shown.
> Before this patch we compared schemas of tables when doing table copy, seem
Yes, you're right, there is no problem now.
In fact, this is a part of work of KUDU-3606, it requires the source and destination tables' column ids are one-to-one corresponding.
We can merge this patch after the main work of KUDU-3606 has been merged.


http://gerrit.cloudera.org:8080/#/c/21695/3//COMMIT_MSG@22
PS3, Line 22: A new flag --show_column_id is also added to
            : indicate whether to show column ids when using
            : 'kudu table describe'.
> nit: This part of change should be put in another patch, because it is for 
Done


http://gerrit.cloudera.org:8080/#/c/21695/3/src/kudu/tools/table_scanner.cc
File src/kudu/tools/table_scanner.cc:

http://gerrit.cloudera.org:8080/#/c/21695/3/src/kudu/tools/table_scanner.cc@446
PS3, Line 446: auto
> nit: Is this change necessary?
Not necessary, but my IDE prompt it. It's a modernize change.



-- 
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: 3
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Fri, 23 Aug 2024 07:23:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3606 [tools] Check column IDs when --strict column id is enabled

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.

Kudu Jenkins 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 6:

Build Started http://jenkins.kudu.apache.org/job/pre_commit/511/


-- 
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: 6
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Fri, 13 Sep 2024 15:32:05 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3606 [tools] Check column IDs when --strict column id is enabled

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

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 7:

(3 comments)

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:     RETURN_NOT_OK(alterer->DropColumn("to_delete_col1")
              :         ->DropColumn("to_delete_col2")
              :         ->DropColumn("to_delete_col3")
              :         ->DropColumn("to_delete_col4")
> Ah, indeed -- I missed that even if it's obvious from lines 1262 -- 1271
Done


http://gerrit.cloudera.org:8080/#/c/21695/6/src/kudu/tools/table_scanner.cc
File src/kudu/tools/table_scanner.cc:

http://gerrit.cloudera.org:8080/#/c/21695/6/src/kudu/tools/table_scanner.cc@496
PS6, Line 496: must be monotonically increasing, b
> must be monotonically increasing
Done


http://gerrit.cloudera.org:8080/#/c/21695/6/src/kudu/tools/table_scanner.cc@497
PS6, Line 497: ecting
> expecting
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: 7
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Wed, 18 Sep 2024 09:30:52 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3606 [tools] Check column IDs when --strict column id is enabled

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin 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 7: Code-Review+2


-- 
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: 7
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Thu, 19 Sep 2024 00:56:59 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3606 [tools] Check column IDs when --strict column id is enabled

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Yingchun Lai has removed a vote on this change.

Change subject: KUDU-3606 [tools] Check column IDs when --strict_column_id is enabled
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
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: deleteVote
Gerrit-Change-Id: I77af7b92b45f3866cc8b699e61b9e71b73ed6c4b
Gerrit-Change-Number: 21695
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>

[kudu-CR] [tools] Copy table with non-continuous column ids

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Hello Mahesh Reddy, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21695

to look at the new patch set (#3).

Change subject: [tools] Copy table with non-continuous column ids
......................................................................

[tools] Copy table with non-continuous column ids

When using the CLI tool 'kudu table copy' to copy
a source table which contains column id holes, to
an existing destination table with the same user
faced schema and with continuous column ids, it
will report an error saying that they have different
schemas, but the difference is not shown.

This patch supplies the error messages.

This patch also adds a flag --strict_column_id to
indicate whether to compare column ids when checking
if the schemas match.

A new flag --show_column_id is also added to
indicate whether to show column ids when using
'kudu table describe'.

Change-Id: I77af7b92b45f3866cc8b699e61b9e71b73ed6c4b
---
M src/kudu/client/schema.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/tool_action_table.cc
4 files changed, 167 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/21695/3
-- 
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: newpatchset
Gerrit-Change-Id: I77af7b92b45f3866cc8b699e61b9e71b73ed6c4b
Gerrit-Change-Number: 21695
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>

[kudu-CR] [tools] Copy table with non-continuous column ids

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/21695

to look at the new patch set (#2).

Change subject: [tools] Copy table with non-continuous column ids
......................................................................

[tools] Copy table with non-continuous column ids

When using the CLI tool 'kudu table copy' to copy
a source table which contains column id holes, to
an existed distination table with the same user
faced schema and with continuous column ids, it
will report an error saying that they have different
schemas, but the difference is not shown.

This patch supplies the error messages.

This patch also adds a flag --strict_column_id to
indicate whether to compare column ids when check
the consistent of shcemas.

A new flag --show_column_id is also added to
indicate whether to show column ids when using
'kudu table describe'.

Change-Id: I77af7b92b45f3866cc8b699e61b9e71b73ed6c4b
---
M src/kudu/client/schema.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/tool_action_table.cc
4 files changed, 168 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/21695/2
-- 
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: newpatchset
Gerrit-Change-Id: I77af7b92b45f3866cc8b699e61b9e71b73ed6c4b
Gerrit-Change-Number: 21695
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)