You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2018/02/20 02:46:45 UTC
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9363
Change subject: KUDU-721: Support range partitions on decimal columns
......................................................................
KUDU-721: Support range partitions on decimal columns
Add the DataType handling to enable range partitions on
decimal columns and a test that fails without the changes.
Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
---
M src/kudu/client/client-test.cc
M src/kudu/common/partition.cc
M src/kudu/util/decimal_util-test.cc
M src/kudu/util/decimal_util.cc
M src/kudu/util/decimal_util.h
5 files changed, 68 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/9363/1
--
To view, visit http://gerrit.cloudera.org:8080/9363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Gerrit-Change-Number: 9363
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@gmail.com>
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9363 )
Change subject: KUDU-721: Support range partitions on decimal columns
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9363/3/python/kudu/client.pyx
File python/kudu/client.pyx:
http://gerrit.cloudera.org:8080/#/c/9363/3/python/kudu/client.pyx@1437
PS3, Line 1437: ``
Is this intended or just a part from some other patch?
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/client/client-test.cc@1662
PS2, Line 1662: TEST_F(ClientTest, TestSwappedRangeBounds) {
: KuduSchemaBuilder builder;
: KuduSchema schema;
: builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
: builder.AddColumn("value")->Type(KuduColumnSchema::INT32)->NotNull();
: ASSERT_OK(builder.Build(&schema));
:
: unique_ptr<KuduPartialRow> lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetInt32("key", 90));
: unique_ptr<KuduPartialRow> upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetInt32("key", -1));
:
: unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
: table_creator->add_range_partition(lower_bound.release(), upper_bound.release(),
: KuduTableCreator::EXCLUSIVE_BOUND,
: KuduTableCreator::INCLUSIVE_BOUND);
:
: Status s = table_creator->table_name("TestSwappedRangeBounds")
: .schema(&schema)
: .num_replicas(1)
: .set_range_partition_columns({ "key" })
: .Create();
:
: ASSERT_TRUE(s.IsInvalidArgument());
: ASSERT_STR_CONTAINS(s.ToString(),
: "Error creating table TestSwappedRangeBounds on the master: "
: "range partition lower bound must be less than the upper bound");
: }
:
:
: TEST_F(ClientTest, TestEqualRangeBounds) {
: KuduSchemaBuilder builder;
: KuduSchema schema;
: builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
: builder.AddColumn("value")->Type(KuduColumnSchema::INT32)->NotNull();
: ASSERT_OK(builder.Build(&schema));
:
: unique_ptr<KuduPartialRow> lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetInt32("key", 10));
: unique_ptr<KuduPartialRow> upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetInt32("key", 10));
:
: unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
: table_creator->add_range_partition(lower_bound.release(), upper_bound.release(),
: KuduTableCreator::EXCLUSIVE_BOUND,
: KuduTableCreator::INCLUSIVE_BOUND);
:
: Status s = table_creator->table_name("TestEqualRangeBounds")
: .schema(&schema)
: .num_replicas(1)
: .set_range_partition_columns({ "key" })
: .Create();
:
: ASSERT_TRUE(s.IsInvalidArgument());
: ASSERT_STR_CONTAINS(s.ToString(),
: "Error creating table TestEqualRangeBounds on the master: "
: "range partition lower bound must be less than the upper bound");
: }
:
: TEST_F(ClientTest, TestMinMaxRangeBounds) {
: KuduSchemaBuilder builder;
: KuduSchema schema;
: builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
: builder.AddColumn("value")->Type(KuduColumnSchema::INT32)->NotNull();
: ASSERT_OK(builder.Build(&schema));
:
: unique_ptr<KuduPartialRow> lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetInt32("key", INT32_MIN));
: unique_ptr<KuduPartialRow> upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetInt32("key", INT32_MAX));
:
: unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
: table_creator->add_range_partition(lower_bound.release(), upper_bound.release(),
: KuduTableCreator::EXCLUSIVE_BOUND,
: KuduTableCreator::INCLUSIVE_BOUND);
:
: ASSERT_OK(table_creator->table_name("TestMinMaxRangeBounds")
: .schema(&schema)
: .num_replicas(1)
: .set_range_partition_columns({ "key" })
: .Create());
: }
> Yeah, this is essentially generic logic. I think a future improvement could
SGTM
--
To view, visit http://gerrit.cloudera.org:8080/9363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Gerrit-Change-Number: 9363
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 21 Feb 2018 18:58:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9363 )
Change subject: KUDU-721: Support range partitions on decimal columns
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9363/3/python/kudu/client.pyx
File python/kudu/client.pyx:
http://gerrit.cloudera.org:8080/#/c/9363/3/python/kudu/client.pyx@1437
PS3, Line 1437: ``
> Is this intended or just a part from some other patch?
oh, my bad. Switching branches to quickly.
--
To view, visit http://gerrit.cloudera.org:8080/9363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Gerrit-Change-Number: 9363
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 21 Feb 2018 19:01:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9363 )
Change subject: KUDU-721: Support range partitions on decimal columns
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc@1639
PS1, Line 1639: schema;
> nit: I don't think we use identifiers like 'x_' for local variables; would
Ah, right. I copied this test from the UnixTimeMicros one above. Will update both.
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc@1643
PS1, Line 1643: ASSERT_O
> why not just ASSERT_OK() ?
Done
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc@1645
PS1, Line 1645: unique_ptr<KuduPartialRow> lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetUnscaledDecimal("key", -1));
: unique_ptr<KuduPartialRow> upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetUnscaledDecimal("key", 99));
> I think it would be nice to add a 'negative' test case (maybe, in some diff
Done
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/common/partition.cc@1043
PS1, Line 1043: const ColumnS
> const reference?
Done
--
To view, visit http://gerrit.cloudera.org:8080/9363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Gerrit-Change-Number: 9363
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 21 Feb 2018 05:01:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9363 )
Change subject: KUDU-721: Support range partitions on decimal columns
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc@1639
PS1, Line 1639: u_schema_
nit: I don't think we use identifiers like 'x_' for local variables; would 'u_schema' suffice here?
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc@1643
PS1, Line 1643: CHECK_OK
why not just ASSERT_OK() ?
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc@1645
PS1, Line 1645: unique_ptr<KuduPartialRow> lower_bound(u_schema_.NewRow());
: ASSERT_OK(lower_bound->SetUnscaledDecimal("key", -1));
: unique_ptr<KuduPartialRow> upper_bound(u_schema_.NewRow());
: ASSERT_OK(upper_bound->SetUnscaledDecimal("key", 99));
I think it would be nice to add a 'negative' test case (maybe, in some different scenario) to test for:
* swapped upper/lower bounds (e.g., upper -1, lower 99)
* bounds that lead to an empty partition (e.g. lower 10, upper 10)
* bounds with MAX and MIN values
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/common/partition.cc@1043
PS1, Line 1043: ColumnSchema
const reference?
--
To view, visit http://gerrit.cloudera.org:8080/9363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Gerrit-Change-Number: 9363
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 20 Feb 2018 22:59:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9363
to look at the new patch set (#3).
Change subject: KUDU-721: Support range partitions on decimal columns
......................................................................
KUDU-721: Support range partitions on decimal columns
Add the DataType handling to enable range partitions on
decimal columns and a test that fails without the changes.
Also adds range partition tests for edge cases where:
* The upper bound is less than than the lower bound
* The upper bound is equal to the lower bound
* The bounds use MIN/MAX values
Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
---
M python/kudu/client.pyx
M src/kudu/client/client-test.cc
M src/kudu/common/partition.cc
M src/kudu/util/decimal_util-test.cc
M src/kudu/util/decimal_util.cc
M src/kudu/util/decimal_util.h
6 files changed, 157 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/9363/3
--
To view, visit http://gerrit.cloudera.org:8080/9363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Gerrit-Change-Number: 9363
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9363
to look at the new patch set (#4).
Change subject: KUDU-721: Support range partitions on decimal columns
......................................................................
KUDU-721: Support range partitions on decimal columns
Add the DataType handling to enable range partitions on
decimal columns and a test that fails without the changes.
Also adds range partition tests for edge cases where:
* The upper bound is less than than the lower bound
* The upper bound is equal to the lower bound
* The bounds use MIN/MAX values
Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
---
M src/kudu/client/client-test.cc
M src/kudu/common/partition.cc
M src/kudu/util/decimal_util-test.cc
M src/kudu/util/decimal_util.cc
M src/kudu/util/decimal_util.h
5 files changed, 156 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/9363/4
--
To view, visit http://gerrit.cloudera.org:8080/9363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Gerrit-Change-Number: 9363
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9363
to look at the new patch set (#2).
Change subject: KUDU-721: Support range partitions on decimal columns
......................................................................
KUDU-721: Support range partitions on decimal columns
Add the DataType handling to enable range partitions on
decimal columns and a test that fails without the changes.
Also adds range partition tests for edge cases where:
* The upper bound is less than than the lower bound
* The upper bound is equal to the lower bound
* The bounds use MIN/MAX values
Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
---
M src/kudu/client/client-test.cc
M src/kudu/common/partition.cc
M src/kudu/util/decimal_util-test.cc
M src/kudu/util/decimal_util.cc
M src/kudu/util/decimal_util.h
5 files changed, 155 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/9363/2
--
To view, visit http://gerrit.cloudera.org:8080/9363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Gerrit-Change-Number: 9363
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9363 )
Change subject: KUDU-721: Support range partitions on decimal columns
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/client/client-test.cc@1662
PS2, Line 1662: TEST_F(ClientTest, TestSwappedRangeBounds) {
: KuduSchemaBuilder builder;
: KuduSchema schema;
: builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
: builder.AddColumn("value")->Type(KuduColumnSchema::INT32)->NotNull();
: ASSERT_OK(builder.Build(&schema));
:
: unique_ptr<KuduPartialRow> lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetInt32("key", 90));
: unique_ptr<KuduPartialRow> upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetInt32("key", -1));
:
: unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
: table_creator->add_range_partition(lower_bound.release(), upper_bound.release(),
: KuduTableCreator::EXCLUSIVE_BOUND,
: KuduTableCreator::INCLUSIVE_BOUND);
:
: Status s = table_creator->table_name("TestSwappedRangeBounds")
: .schema(&schema)
: .num_replicas(1)
: .set_range_partition_columns({ "key" })
: .Create();
:
: ASSERT_TRUE(s.IsInvalidArgument());
: ASSERT_STR_CONTAINS(s.ToString(),
: "Error creating table TestSwappedRangeBounds on the master: "
: "range partition lower bound must be less than the upper bound");
: }
:
:
: TEST_F(ClientTest, TestEqualRangeBounds) {
: KuduSchemaBuilder builder;
: KuduSchema schema;
: builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
: builder.AddColumn("value")->Type(KuduColumnSchema::INT32)->NotNull();
: ASSERT_OK(builder.Build(&schema));
:
: unique_ptr<KuduPartialRow> lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetInt32("key", 10));
: unique_ptr<KuduPartialRow> upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetInt32("key", 10));
:
: unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
: table_creator->add_range_partition(lower_bound.release(), upper_bound.release(),
: KuduTableCreator::EXCLUSIVE_BOUND,
: KuduTableCreator::INCLUSIVE_BOUND);
:
: Status s = table_creator->table_name("TestEqualRangeBounds")
: .schema(&schema)
: .num_replicas(1)
: .set_range_partition_columns({ "key" })
: .Create();
:
: ASSERT_TRUE(s.IsInvalidArgument());
: ASSERT_STR_CONTAINS(s.ToString(),
: "Error creating table TestEqualRangeBounds on the master: "
: "range partition lower bound must be less than the upper bound");
: }
:
: TEST_F(ClientTest, TestMinMaxRangeBounds) {
: KuduSchemaBuilder builder;
: KuduSchema schema;
: builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
: builder.AddColumn("value")->Type(KuduColumnSchema::INT32)->NotNull();
: ASSERT_OK(builder.Build(&schema));
:
: unique_ptr<KuduPartialRow> lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetInt32("key", INT32_MIN));
: unique_ptr<KuduPartialRow> upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetInt32("key", INT32_MAX));
:
: unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
: table_creator->add_range_partition(lower_bound.release(), upper_bound.release(),
: KuduTableCreator::EXCLUSIVE_BOUND,
: KuduTableCreator::INCLUSIVE_BOUND);
:
: ASSERT_OK(table_creator->table_name("TestMinMaxRangeBounds")
: .schema(&schema)
: .num_replicas(1)
: .set_range_partition_columns({ "key" })
: .Create());
: }
> Thank you for adding those new scenarios.
Yeah, this is essentially generic logic. I think a future improvement could be to write a templated test that uses all supported types. I would do that in a follow up patch given it's just a test coverage gap in general, not specific to this patch.
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/common/partition.cc@1005
PS2, Line 1005: const ColumnSche
> nit: I missed that in the first revision, but this could be const reference
Done
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/util/decimal_util.h
File src/kudu/util/decimal_util.h:
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/util/decimal_util.h@60
PS2, Line 60: n
> tiny nit: add a stop (period) in the end
Done
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/util/decimal_util.h@64
PS2, Line 64: n
> tiny nit: add a stop (period) in the end
Done
--
To view, visit http://gerrit.cloudera.org:8080/9363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Gerrit-Change-Number: 9363
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 21 Feb 2018 18:54:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9363 )
Change subject: KUDU-721: Support range partitions on decimal columns
......................................................................
KUDU-721: Support range partitions on decimal columns
Add the DataType handling to enable range partitions on
decimal columns and a test that fails without the changes.
Also adds range partition tests for edge cases where:
* The upper bound is less than than the lower bound
* The upper bound is equal to the lower bound
* The bounds use MIN/MAX values
Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Reviewed-on: http://gerrit.cloudera.org:8080/9363
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/client/client-test.cc
M src/kudu/common/partition.cc
M src/kudu/util/decimal_util-test.cc
M src/kudu/util/decimal_util.cc
M src/kudu/util/decimal_util.h
5 files changed, 156 insertions(+), 10 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Gerrit-Change-Number: 9363
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9363 )
Change subject: KUDU-721: Support range partitions on decimal columns
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Gerrit-Change-Number: 9363
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 21 Feb 2018 19:15:48 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9363 )
Change subject: KUDU-721: Support range partitions on decimal columns
......................................................................
Patch Set 2: Code-Review+1
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc@1639
PS1, Line 1639: schema;
> Ah, right. I copied this test from the UnixTimeMicros one above. Will updat
Thanks!
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/client/client-test.cc@1662
PS2, Line 1662: TEST_F(ClientTest, TestSwappedRangeBounds) {
: KuduSchemaBuilder builder;
: KuduSchema schema;
: builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
: builder.AddColumn("value")->Type(KuduColumnSchema::INT32)->NotNull();
: ASSERT_OK(builder.Build(&schema));
:
: unique_ptr<KuduPartialRow> lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetInt32("key", 90));
: unique_ptr<KuduPartialRow> upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetInt32("key", -1));
:
: unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
: table_creator->add_range_partition(lower_bound.release(), upper_bound.release(),
: KuduTableCreator::EXCLUSIVE_BOUND,
: KuduTableCreator::INCLUSIVE_BOUND);
:
: Status s = table_creator->table_name("TestSwappedRangeBounds")
: .schema(&schema)
: .num_replicas(1)
: .set_range_partition_columns({ "key" })
: .Create();
:
: ASSERT_TRUE(s.IsInvalidArgument());
: ASSERT_STR_CONTAINS(s.ToString(),
: "Error creating table TestSwappedRangeBounds on the master: "
: "range partition lower bound must be less than the upper bound");
: }
:
:
: TEST_F(ClientTest, TestEqualRangeBounds) {
: KuduSchemaBuilder builder;
: KuduSchema schema;
: builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
: builder.AddColumn("value")->Type(KuduColumnSchema::INT32)->NotNull();
: ASSERT_OK(builder.Build(&schema));
:
: unique_ptr<KuduPartialRow> lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetInt32("key", 10));
: unique_ptr<KuduPartialRow> upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetInt32("key", 10));
:
: unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
: table_creator->add_range_partition(lower_bound.release(), upper_bound.release(),
: KuduTableCreator::EXCLUSIVE_BOUND,
: KuduTableCreator::INCLUSIVE_BOUND);
:
: Status s = table_creator->table_name("TestEqualRangeBounds")
: .schema(&schema)
: .num_replicas(1)
: .set_range_partition_columns({ "key" })
: .Create();
:
: ASSERT_TRUE(s.IsInvalidArgument());
: ASSERT_STR_CONTAINS(s.ToString(),
: "Error creating table TestEqualRangeBounds on the master: "
: "range partition lower bound must be less than the upper bound");
: }
:
: TEST_F(ClientTest, TestMinMaxRangeBounds) {
: KuduSchemaBuilder builder;
: KuduSchema schema;
: builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
: builder.AddColumn("value")->Type(KuduColumnSchema::INT32)->NotNull();
: ASSERT_OK(builder.Build(&schema));
:
: unique_ptr<KuduPartialRow> lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetInt32("key", INT32_MIN));
: unique_ptr<KuduPartialRow> upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetInt32("key", INT32_MAX));
:
: unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
: table_creator->add_range_partition(lower_bound.release(), upper_bound.release(),
: KuduTableCreator::EXCLUSIVE_BOUND,
: KuduTableCreator::INCLUSIVE_BOUND);
:
: ASSERT_OK(table_creator->table_name("TestMinMaxRangeBounds")
: .schema(&schema)
: .num_replicas(1)
: .set_range_partition_columns({ "key" })
: .Create());
: }
Thank you for adding those new scenarios.
Originally I though about adding those just for the decimal types, but if that's just some common stuff for any type then it looks good to me.
BTW, did you verified that it works the same way for the decimal types as well?
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/common/partition.cc@1005
PS2, Line 1005: ColumnSchema col
nit: I missed that in the first revision, but this could be const reference as well, if I'm not mistaken.
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/util/decimal_util.h
File src/kudu/util/decimal_util.h:
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/util/decimal_util.h@60
PS2, Line 60: n
tiny nit: add a stop (period) in the end
http://gerrit.cloudera.org:8080/#/c/9363/2/src/kudu/util/decimal_util.h@64
PS2, Line 64: n
tiny nit: add a stop (period) in the end
--
To view, visit http://gerrit.cloudera.org:8080/9363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Gerrit-Change-Number: 9363
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 21 Feb 2018 18:44:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9363 )
Change subject: KUDU-721: Support range partitions on decimal columns
......................................................................
Patch Set 1:
LGTM with Alexey's points addressed
--
To view, visit http://gerrit.cloudera.org:8080/9363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917
Gerrit-Change-Number: 9363
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 20 Feb 2018 23:07:43 +0000
Gerrit-HasComments: No