[kudu-CR] KUDU-1945 Update default range partition key
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/20219 )
Change subject: KUDU-1945 Update default range partition key
..
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20219/3/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/20219/3/src/kudu/common/partition-test.cc@2333
PS3, Line 2333: ASSERT_EQ("HASH (c1) PARTITIONS 2, RANGE (c1)"
> Doesn't this look strange and quite unexpected that the resulted partitioni
OK, regardless this feature, it's not pertaining to this changelist :)
--
To view, visit http://gerrit.cloudera.org:8080/20219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc
Gerrit-Change-Number: 20219
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Comment-Date: Thu, 20 Jul 2023 20:17:15 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update default range partition key
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20219 ) Change subject: KUDU-1945 Update default range partition key .. KUDU-1945 Update default range partition key The default range partition key includes all the columns of the primary key. We should not include auto incrementing column as this is not expected to be a part of the partition key by design. Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc Reviewed-on: http://gerrit.cloudera.org:8080/20219 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/common/partition-test.cc M src/kudu/common/partition.cc 2 files changed, 23 insertions(+), 2 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/20219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc Gerrit-Change-Number: 20219 Gerrit-PatchSet: 5 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] KUDU-1945 Update default range partition key
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/20219 )
Change subject: KUDU-1945 Update default range partition key
..
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20219/3/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/20219/3/src/kudu/common/partition-test.cc@2333
PS3, Line 2333: ASSERT_EQ("HASH (c1) PARTITIONS 2, RANGE (c1)"
Doesn't this look strange and quite unexpected that the resulted partitioning
is a two-tiered one, when the request was to create one-tiered hash
partitioning?
http://gerrit.cloudera.org:8080/#/c/20219/1/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/20219/1/src/kudu/common/partition.cc@277
PS1, Line 277: if (schema.auto_incrementing_col_idx() != column_idx) {
> Yes, I manually looked through all the occurrences of RangeSchema range_sch
Thanks you for the clarification.
--
To view, visit http://gerrit.cloudera.org:8080/20219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc
Gerrit-Change-Number: 20219
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Comment-Date: Thu, 20 Jul 2023 17:22:33 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update default range partition key
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20219 ) Change subject: KUDU-1945 Update default range partition key .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/20219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc Gerrit-Change-Number: 20219 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Thu, 20 Jul 2023 16:27:24 + Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Update default range partition key
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20219 ) Change subject: KUDU-1945 Update default range partition key .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/20219/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20219/2//COMMIT_MSG@7 PS2, Line 7: default rang > nit: default range Done http://gerrit.cloudera.org:8080/#/c/20219/2//COMMIT_MSG@9 PS2, Line 9: columns > nit: columns Done http://gerrit.cloudera.org:8080/#/c/20219/2//COMMIT_MSG@11 PS2, Line 11: not expected to > nit: not expected to Done -- To view, visit http://gerrit.cloudera.org:8080/20219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc Gerrit-Change-Number: 20219 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Thu, 20 Jul 2023 07:21:23 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update default range partition key
Hello Mahesh Reddy, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20219 to look at the new patch set (#3). Change subject: KUDU-1945 Update default range partition key .. KUDU-1945 Update default range partition key The default range partition key includes all the columns of the primary key. We should not include auto incrementing column as this is not expected to be a part of the partition key by design. Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc --- M src/kudu/common/partition-test.cc M src/kudu/common/partition.cc 2 files changed, 23 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/20219/3 -- To view, visit http://gerrit.cloudera.org:8080/20219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc Gerrit-Change-Number: 20219 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] KUDU-1945 Update Default Range partition Key
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20219 ) Change subject: KUDU-1945 Update Default Range partition Key .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/20219/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20219/2//COMMIT_MSG@7 PS2, Line 7: Default Rang nit: default range http://gerrit.cloudera.org:8080/#/c/20219/2//COMMIT_MSG@9 PS2, Line 9: columsn nit: columns http://gerrit.cloudera.org:8080/#/c/20219/2//COMMIT_MSG@11 PS2, Line 11: expected to not nit: not expected to -- To view, visit http://gerrit.cloudera.org:8080/20219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc Gerrit-Change-Number: 20219 Gerrit-PatchSet: 2 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Wed, 19 Jul 2023 03:52:12 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update Default Range partition Key
Hello Mahesh Reddy, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20219 to look at the new patch set (#2). Change subject: KUDU-1945 Update Default Range partition Key .. KUDU-1945 Update Default Range partition Key The default range partition key includes all the columsn of the primary key. We should not include auto incrementing column as this is expected to not be a part of the partition key by design. Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc --- M src/kudu/common/partition-test.cc M src/kudu/common/partition.cc 2 files changed, 23 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/20219/2 -- To view, visit http://gerrit.cloudera.org:8080/20219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc Gerrit-Change-Number: 20219 Gerrit-PatchSet: 2 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] KUDU-1945 Update Default Range partition Key
Abhishek Chennaka has posted comments on this change. (
http://gerrit.cloudera.org:8080/20219 )
Change subject: KUDU-1945 Update Default Range partition Key
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20219/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20219/1//COMMIT_MSG@9
PS1, Line 9: primary keys
> columns of the primary key?
Done
http://gerrit.cloudera.org:8080/#/c/20219/1/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/20219/1/src/kudu/common/partition-test.cc@2316
PS1, Line 2316: TEST_F(PartitionTest, PartitionKeyWithAutoIncrementingColumn) {
> It would be great to provide a small blurb describing what is the essence o
Yes, it would fail as we would end up creating the below partition schema:
HASH (c1) PARTITIONS 2, RANGE (c1,auto_incrementing_id).
I'll add a comment above.
http://gerrit.cloudera.org:8080/#/c/20219/1/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/20219/1/src/kudu/common/partition.cc@277
PS1, Line 277: if (schema.auto_incrementing_col_idx() != column_idx) {
> BTW, have you checked out other places in the code where RangeSchema::colum
Yes, I manually looked through all the occurrences of RangeSchema range_schema_
member variable in partition.cc to understand if this change breaks anything
and found nothing including PartitionSchema::PartitionMayContainRow.
To give more context, this happens when there is no range partition schema set.
Looks like it was assumed to use all of the PK to create a range partition
seemed like a reasonable approach previously. But that doesn't work with
auto-incrementing columns as they are not meant to be partition keys. And there
is no assumption that range partition key should have all the primary key
columns present as it is a user defined field.
I also found that if there is no range schema present during a table DDL,
impala seems to be setting it to null explicitly and we would not have this
behavior over there.
--
To view, visit http://gerrit.cloudera.org:8080/20219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc
Gerrit-Change-Number: 20219
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Comment-Date: Wed, 19 Jul 2023 01:40:00 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update Default Range partition Key
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/20219 )
Change subject: KUDU-1945 Update Default Range partition Key
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20219/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20219/1//COMMIT_MSG@9
PS1, Line 9: primary keys
columns of the primary key?
http://gerrit.cloudera.org:8080/#/c/20219/1/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/20219/1/src/kudu/common/partition-test.cc@2316
PS1, Line 2316: TEST_F(PartitionTest, PartitionKeyWithAutoIncrementingColumn) {
It would be great to provide a small blurb describing what is the essence of
this scenario.
Is my understanding correct that this test would fail without the corresponding
change in partition.cc that's a part of this patch?
http://gerrit.cloudera.org:8080/#/c/20219/1/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/20219/1/src/kudu/common/partition.cc@277
PS1, Line 277: if (schema.auto_incrementing_col_idx() != column_idx) {
BTW, have you checked out other places in the code where
RangeSchema::column_ids field is populated?
I'm also curious whether it's necessary to revise other places where that field
is being used. E.g., does PartitionSchema::PartitionMayContainRow() needs to
be updated as well?
--
To view, visit http://gerrit.cloudera.org:8080/20219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc
Gerrit-Change-Number: 20219
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Comment-Date: Wed, 19 Jul 2023 01:10:25 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update Default Range partition Key
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20219 ) Change subject: KUDU-1945 Update Default Range partition Key .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/20219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc Gerrit-Change-Number: 20219 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 18 Jul 2023 20:51:51 + Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Update Default Range partition Key
Abhishek Chennaka has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20219 Change subject: KUDU-1945 Update Default Range partition Key .. KUDU-1945 Update Default Range partition Key The default range partition key includes all the primary keys. We should not include auto incrementing column as this is expected to not be a part of the partition key by design. Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc --- M src/kudu/common/partition-test.cc M src/kudu/common/partition.cc 2 files changed, 21 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/20219/1 -- To view, visit http://gerrit.cloudera.org:8080/20219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I89818ceb261064369a63712f6c093f41e57ca5cc Gerrit-Change-Number: 20219 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Chennaka
