[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17161 ) Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. KUDU-2671: No-op ClientServerMapping if only one schema exists. For the new field of PartitionSchema, a client schema is necessary for RowOperationsPBDecoder to populate the new field. Not all call sites for PartitionSchema::FromPB() have access to an explicit client schema. This patch allows the client schema to have column IDs if it's equal to the tablet schema and no-ops the ClientServerMapping in this case. If we don't have access to a client schema, we don't need a mapping since we only have one schema. Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Reviewed-on: http://gerrit.cloudera.org:8080/17161 Reviewed-by: Andrew Wong Tested-by: Andrew Wong --- M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h 3 files changed, 43 insertions(+), 14 deletions(-) Approvals: Andrew Wong: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 7 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17161 ) Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. Patch Set 6: Verified+1 Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 6 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Fri, 12 Mar 2021 01:05:08 + Gerrit-HasComments: No
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Andrew Wong has removed a vote on this change. Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 6 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17161 ) Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. Patch Set 6: > Patch Set 6: Verified-1 > > Build Failed > > http://jenkins.kudu.apache.org/job/kudu-gerrit/23475/ : FAILURE Looks like the test failure is unrelated. -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 6 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Fri, 12 Mar 2021 01:03:02 + Gerrit-HasComments: No
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17161 to look at the new patch set (#6). Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. KUDU-2671: No-op ClientServerMapping if only one schema exists. For the new field of PartitionSchema, a client schema is necessary for RowOperationsPBDecoder to populate the new field. Not all call sites for PartitionSchema::FromPB() have access to an explicit client schema. This patch allows the client schema to have column IDs if it's equal to the tablet schema and no-ops the ClientServerMapping in this case. If we don't have access to a client schema, we don't need a mapping since we only have one schema. Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e --- M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h 3 files changed, 43 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/17161/6 -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 6 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17161 ) Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/17161/4/src/kudu/common/row_operations.h File src/kudu/common/row_operations.h: http://gerrit.cloudera.org:8080/#/c/17161/4/src/kudu/common/row_operations.h@140 PS4, Line 140: 'client_col_id > nit: following suit with other comments, could you wrap variable/arg names Done -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 5 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Thu, 11 Mar 2021 01:14:25 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17161 to look at the new patch set (#5). Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. KUDU-2671: No-op ClientServerMapping if only one schema exists. For the new field of PartitionSchema, a client schema is necessary for RowOperationsPBDecoder to populate the new field. Not all call sites for PartitionSchema::FromPB() have access to an explicit client schema. This patch allows the client schema to have column IDs if it's equal to the tablet schema and no-ops the ClientServerMapping in this case. If we don't have access to a client schema, we don't need a mapping since we only have one schema. Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e --- M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h 3 files changed, 43 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/17161/5 -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 5 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17161 ) Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. Patch Set 4: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/17161/4/src/kudu/common/row_operations.h File src/kudu/common/row_operations.h: http://gerrit.cloudera.org:8080/#/c/17161/4/src/kudu/common/row_operations.h@140 PS4, Line 140: client_col_idx nit: following suit with other comments, could you wrap variable/arg names with ''s? That'd make it a bit more obvious that you're referring them specifically. -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 4 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Thu, 11 Mar 2021 00:59:04 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17161 to look at the new patch set (#4). Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. KUDU-2671: No-op ClientServerMapping if only one schema exists. For the new field of PartitionSchema, a client schema is necessary for RowOperationsPBDecoder to populate the new field. Not all call sites for PartitionSchema::FromPB() have access to an explicit client schema. This patch allows the client schema to have column IDs if it's equal to the tablet schema and no-ops the ClientServerMapping in this case. If we don't have access to a client schema, we don't need a mapping since we only have one schema. Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e --- M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h 3 files changed, 43 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/17161/4 -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 4 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17161 ) Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/17161/3/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/17161/3/src/kudu/common/row_operations.cc@449 PS3, Line 449: size_t tablet_col_idx = client_col_idx; > nit: would it make sense to separate this into an utility method? I considered this as well, I can make that change. http://gerrit.cloudera.org:8080/#/c/17161/3/src/kudu/common/row_operations.cc@683 PS3, Line 683: mapping > BTW, what's the motivation behind keeping this as a location variable and p To answer the first part, I didn't consider the tradeoff between keeping it as a location variable or turning it into a member of this class since the scope of this patch was to just allow the client schema to have column IDs if its the same schema object as the tablet schema. I could turn the mapping object into a member of RowOperationsPBDecoder, of course it would involve touching more areas of the code. I'm ok keeping it as is, but if you feel strongly about it I can make 'mapping' a member of this class. -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 3 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Wed, 10 Mar 2021 03:19:18 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17161 ) Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/17161/3/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/17161/3/src/kudu/common/row_operations.cc@449 PS3, Line 449: size_t tablet_col_idx = client_col_idx; nit: would it make sense to separate this into an utility method? http://gerrit.cloudera.org:8080/#/c/17161/3/src/kudu/common/row_operations.cc@683 PS3, Line 683: mapping BTW, what's the motivation behind keeping this as a location variable and passing it along as a parameter? Would it make sense to turn this into a member of this class? -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 3 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Wed, 10 Mar 2021 00:31:22 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Mahesh Reddy has posted comments on this change. (
http://gerrit.cloudera.org:8080/17161 )
Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists.
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17161/2/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/17161/2/src/kudu/common/row_operations.cc@682
PS2, Line 682: if (client_schema_ != tablet_schema_) {
:
RETURN_NOT_OK(client_schema_->GetProjectionMapping(*tablet_schema_, ));
: DCHECK_EQ(mapping.num_mapped(), client_schema_->num_columns(
> nit: how about instead we just put this into the if block below? Then we wo
Done
--
To view, visit http://gerrit.cloudera.org:8080/17161
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e
Gerrit-Change-Number: 17161
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Comment-Date: Tue, 09 Mar 2021 21:50:47 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Hello Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17161 to look at the new patch set (#3). Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. KUDU-2671: No-op ClientServerMapping if only one schema exists. For the new field of PartitionSchema, a client schema is necessary for RowOperationsPBDecoder to populate the new field. Not all call sites for PartitionSchema::FromPB() have access to an explicit client schema. This patch allows the client schema to have column IDs if it's equal to the tablet schema and no-ops the ClientServerMapping in this case. If we don't have access to a client schema, we don't need a mapping since we only have one schema. Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e --- M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h 3 files changed, 44 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/17161/3 -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 3 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17161 ) Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/17161/2/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/17161/2/src/kudu/common/row_operations.cc@682 PS2, Line 682: RETURN_NOT_OK(client_schema_->GetProjectionMapping(*tablet_schema_, : client_schema_ == tablet_schema_, : )); nit: how about instead we just put this into the if block below? Then we wouldn't have to touch GetProjectionMapping() at all -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 2 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 09 Mar 2021 07:10:57 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17161 ) Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc File src/kudu/common/row_operations-test.cc: http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@761 PS1, Line 761: chema_as_cl > Right, the distinction between using column IDs vs not. Ack http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.cc@221 PS1, Line 221: > I don't think an enum makes too much sense. Adding docs to the constructor' Done -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 2 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 09 Mar 2021 06:35:02 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Hello Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17161 to look at the new patch set (#2). Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. KUDU-2671: No-op ClientServerMapping if only one schema exists. For the new field of PartitionSchema, a client schema is necessary for RowOperationsPBDecoder to populate the new field. Not all call sites for PartitionSchema::FromPB() have access to an explicit client schema. This patch allows the client schema to have column IDs if it's equal to the tablet schema and no-ops the ClientServerMapping in this case. If we don't have access to a client schema, we don't need a mapping since we only have one schema. Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e --- M src/kudu/common/row.h M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h M src/kudu/common/schema.h 5 files changed, 56 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/17161/2 -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 2 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17161 ) Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc File src/kudu/common/row_operations-test.cc: http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@761 PS1, Line 761: const bool& > Not entirely sure what distinction you're referring to here. Are you talkin Right, the distinction between using column IDs vs not. http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.cc@221 PS1, Line 221: client_schema->Equals(*tablet_schema, Schema::COMPARE_ALL) > That's a good point, the main use case is when the same schema object is us I don't think an enum makes too much sense. Adding docs to the constructor's comment or the comments around the definitions of client_schema_ and tablet_schema_ should suffice. -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 1 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 09 Mar 2021 03:25:07 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Mahesh Reddy has posted comments on this change. (
http://gerrit.cloudera.org:8080/17161 )
Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists.
..
Patch Set 1:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc
File src/kudu/common/row_operations-test.cc:
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@761
PS1, Line 761: const bool&
> nit: just use bool, or an enum
Not entirely sure what distinction you're referring to here. Are you talking
about the impact that the bool being true/false has on the client schema?
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@761
PS1, Line 761: client_has_ids
> nit: maybe call this "use_client_schema_as_tablet_schema" or something? Or
Ack
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@794
PS1, Line 794: _schema
> nit: would it be more representative of your expected usages to do somethin
sure, although '_schema' and '' would be flipped in your example.
Can get rid of the if check on L 791 too.
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@937
PS1, Line 937: TEST_F(RowOperationsTest, SplitKeysRoundTrip) {
> nit: it's not clear what exactly this is testing. Could you add a descripti
Ack
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.h@176
PS1, Line 176: bool
> nit: for readability, we typically prefer using enums instead of booleans t
Ack
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.cc@221
PS1, Line 221: client_schema->Equals(*tablet_schema, Schema::COMPARE_ALL)
> Won't the main usage here use the same schema object? If so, can't we just
That's a good point, the main use case is when the same schema object is used.
My only worry with this approach is that it's more generic than comparing two
schemas' fields, but since the end user is us, documentation should be enough
of a safeguard.
Do you think it would be more clear to still have an enum field that reflects
this? It also gives us a place to include documentation of only allowing client
schema to have column IDs when it's the same object.
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/schema.h@874
PS1, Line 874: const bool&
> nit: primitive types don't need const&.
Ack
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/schema.h@874
PS1, Line 874: client_equals_to_tablet_schema,
> nit: document what this does and any expectations users should have in sett
Ack
--
To view, visit http://gerrit.cloudera.org:8080/17161
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e
Gerrit-Change-Number: 17161
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Comment-Date: Tue, 09 Mar 2021 03:09:17 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/17161 )
Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists.
..
Patch Set 1:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc
File src/kudu/common/row_operations-test.cc:
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@761
PS1, Line 761: client_has_ids
nit: maybe call this "use_client_schema_as_tablet_schema" or something? Or
something along those lines (i know it's a mouthful)
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@761
PS1, Line 761: const bool&
nit: just use bool, or an enum
Also can you describe why the distinction is important as a comment?
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@794
PS1, Line 794: _schema
nit: would it be more representative of your expected usages to do something
like 'use_client_schema_as_tablet_schema ? _schema : '?
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@937
PS1, Line 937: TEST_F(RowOperationsTest, SplitKeysRoundTrip) {
nit: it's not clear what exactly this is testing. Could you add a description?
And maybe allude to how it's exercising specific codepaths.
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.h@176
PS1, Line 176: bool
nit: for readability, we typically prefer using enums instead of booleans to
define a mode of operation. How about defining some
enum DecoderSchemaUsage {
kClientSchemaNoColIds,
kClientSchemaEqualsTabletSchema,
};
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.cc@221
PS1, Line 221: client_schema->Equals(*tablet_schema, Schema::COMPARE_ALL)
Won't the main usage here use the same schema object? If so, can't we just use
pointer comparison? Then we wouldn't even need to define this bool (or enum, as
mentioned in my other comment) -- we could just use
if (client_schema_ != tablet_schema_) { ...
everywhere. And of course we should also then document that it is the only time
it's acceptable for the client schema to have column IDs.
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/schema.h@874
PS1, Line 874: client_equals_to_tablet_schema,
nit: document what this does and any expectations users should have in setting
it.
http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/schema.h@874
PS1, Line 874: const bool&
nit: primitive types don't need const&.
--
To view, visit http://gerrit.cloudera.org:8080/17161
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e
Gerrit-Change-Number: 17161
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 09 Mar 2021 01:22:54 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Mahesh Reddy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17161 Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. KUDU-2671: No-op ClientServerMapping if only one schema exists. For the new field of PartitionSchema, a client schema is necessary for RowOperationsPBDecoder to populate the new field. Not all call sites for PartitionSchema::FromPB() have access to an explicit client schema. This patch allows the client schema to have column IDs if it's equal to the tablet schema and no-ops the ClientServerMapping in this case. If we don't have access to a client schema, we don't need a mapping since we only have one schema. Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e --- M src/kudu/common/row.h M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h M src/kudu/common/schema.h 5 files changed, 58 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/17161/1 -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 1 Gerrit-Owner: Mahesh Reddy
