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 <granthe...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 21 Feb 2018 18:44:45 +0000 Gerrit-HasComments: Yes
