This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch branch-1.18.x in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 383731148ee0ecc768cdeb7200b4bdd5617517a7 Author: Alexey Serbin <[email protected]> AuthorDate: Mon Jun 2 17:31:46 2025 -0700 [common] introduce ColumnSchemaBuilder My in-progress patches to introduce array data type require updates on ColumnSchema. However, the number of parameters-by-default for ColumnSchema constructor is unmaintainable already. Consequently, the are too many unrelated parameters specified at call sites, making it hard to read and comprehend, and even more so if trying to make any updates. This changelist addresses these issues by introducing ColumnSchemaBuilder helper (similar to KuduClientBuilder, etc.). I also moved the full-fledged constructor of the ColumnSchema class into the private section to encourage using the simpler one and switch to ColumnSchemaBuilder for more advanced use cases. Correspondingly, I updated all the call sites to switch to ColumnSchemaBuilder where necessary. I removed the usage of std::optional for ColumnSchemaFromPB(), switching it to ColumnSchemaBuilder and renaming the helper function into ColumnSchemaBuilderFromPB(). In addition, a new enumeration ColumnSchema::Nullability has been added to specify NULLABLE and NOT_NULL instead of boolean parameter in the simpler ColumnSchema's constructor. This patch doesn't contain any functional modifications. Change-Id: I1c21b459b24675d0702c91add27950d91975fad9 Reviewed-on: http://gerrit.cloudera.org:8080/22976 Tested-by: Kudu Jenkins Reviewed-by: Abhishek Chennaka <[email protected]> (cherry picked from commit fe0410d03bfda8487cd11b13a068d403ba6a46cc) Conflicts: src/kudu/master/catalog_manager-test.cc Reviewed-on: http://gerrit.cloudera.org:8080/23998 Tested-by: Alexey Serbin <[email protected]> --- src/kudu/client/scan_configuration.cc | 12 +- src/kudu/client/schema.cc | 18 +- src/kudu/codegen/codegen-test.cc | 38 ++- src/kudu/common/column_predicate-test.cc | 63 ++-- src/kudu/common/generic_iterators-test.cc | 10 +- src/kudu/common/key_util-test.cc | 10 +- src/kudu/common/partial_row-test.cc | 18 +- src/kudu/common/partition-test.cc | 16 +- src/kudu/common/row_operations-test.cc | 59 ++-- src/kudu/common/scan_spec-test.cc | 7 +- src/kudu/common/schema-test.cc | 114 ++++--- src/kudu/common/schema.cc | 78 +---- src/kudu/common/schema.h | 348 ++++++++++++++++----- src/kudu/common/wire_protocol-test-util.h | 7 +- src/kudu/common/wire_protocol-test.cc | 126 +++++--- src/kudu/common/wire_protocol.cc | 54 ++-- src/kudu/common/wire_protocol.h | 10 +- .../integration-tests/auto_incrementing-itest.cc | 7 +- src/kudu/integration-tests/create-table-itest.cc | 4 +- src/kudu/master/catalog_manager.cc | 11 +- src/kudu/master/master-test.cc | 2 +- src/kudu/tablet/all_types-scan-correctness-test.cc | 48 ++- src/kudu/tablet/cfile_set-test.cc | 15 +- src/kudu/tablet/compaction-test.cc | 12 +- src/kudu/tablet/delta_compaction-test.cc | 12 +- src/kudu/tablet/diff_scan-test.cc | 16 +- src/kudu/tablet/diskrowset-test.cc | 7 +- src/kudu/tablet/key_value_test_schema.h | 2 +- src/kudu/tablet/memrowset-test.cc | 15 +- src/kudu/tablet/tablet-decoder-eval-test.cc | 21 +- src/kudu/tablet/tablet-pushdown-test.cc | 4 +- src/kudu/tablet/tablet-schema-test.cc | 13 +- src/kudu/tablet/tablet-test-base.h | 19 +- src/kudu/tablet/tablet-test-util.h | 7 +- src/kudu/tablet/tablet-test.cc | 7 +- src/kudu/tablet/tablet_auto_incrementing-test.cc | 8 +- src/kudu/tools/kudu-tool-test.cc | 16 +- src/kudu/tserver/tablet_server-test.cc | 39 ++- .../tserver/tablet_server_authorization-test.cc | 27 +- src/kudu/tserver/tablet_service.cc | 26 +- 40 files changed, 821 insertions(+), 505 deletions(-) diff --git a/src/kudu/client/scan_configuration.cc b/src/kudu/client/scan_configuration.cc index f0b725e19..b3f32df74 100644 --- a/src/kudu/client/scan_configuration.cc +++ b/src/kudu/client/scan_configuration.cc @@ -241,14 +241,10 @@ Status ScanConfiguration::AddIsDeletedColumn() { // Add the IS_DELETED virtual column to the list of projected columns. bool read_default = false; - ColumnSchema is_deleted(col_name, - IS_DELETED, - /*is_nullable=*/false, - /*is_immutable=*/false, - /*is_auto_incrementing=*/false, - &read_default); - cols.emplace_back(std::move(is_deleted)); - + cols.emplace_back(ColumnSchemaBuilder() + .name(col_name) + .type(IS_DELETED) + .read_default(&read_default)); return CreateProjection(cols); } diff --git a/src/kudu/client/schema.cc b/src/kudu/client/schema.cc index dbf59d1d8..df9630a67 100644 --- a/src/kudu/client/schema.cc +++ b/src/kudu/client/schema.cc @@ -830,12 +830,18 @@ KuduColumnSchema::KuduColumnSchema(const string &name, type_attr_private.precision = type_attributes.precision(); type_attr_private.scale = type_attributes.scale(); type_attr_private.length = type_attributes.length(); - col_ = new ColumnSchema(name, ToInternalDataType(type, type_attributes), - is_nullable, - is_immutable, - is_auto_incrementing, - default_value, default_value, attr_private, - type_attr_private, comment); + col_ = ColumnSchemaBuilder() + .name(name) + .type(ToInternalDataType(type, type_attributes)) + .nullable(is_nullable) + .immutable(is_immutable) + .auto_incrementing(is_auto_incrementing) + .read_default(default_value) + .write_default(default_value) + .storage_attributes(attr_private) + .type_attributes(type_attr_private) + .comment(comment) + .New().release(); } KuduColumnSchema::KuduColumnSchema(const KuduColumnSchema& other) diff --git a/src/kudu/codegen/codegen-test.cc b/src/kudu/codegen/codegen-test.cc index c7438c24c..a1eda8c8b 100644 --- a/src/kudu/codegen/codegen-test.cc +++ b/src/kudu/codegen/codegen-test.cc @@ -74,21 +74,37 @@ class CodegenTest : public KuduTest { // Set the initial Arena size as small as possible to catch errors during relocation. projections_mem_(16) { // Create the base schema. - vector<ColumnSchema> cols = { ColumnSchema("key ", UINT64, false), - ColumnSchema("int32 ", INT32, false), - ColumnSchema("int32-null-val", INT32, true), - ColumnSchema("int32-null ", INT32, true), - ColumnSchema("str32 ", STRING, false), - ColumnSchema("str32-null-val", STRING, true), - ColumnSchema("str32-null ", STRING, true) }; + vector<ColumnSchema> cols = { + ColumnSchema("key ", UINT64, ColumnSchema::NOT_NULL), + ColumnSchema("int32 ", INT32, ColumnSchema::NOT_NULL), + ColumnSchema("int32-null-val", INT32, ColumnSchema::NULLABLE), + ColumnSchema("int32-null ", INT32, ColumnSchema::NULLABLE), + ColumnSchema("str32 ", STRING, ColumnSchema::NOT_NULL), + ColumnSchema("str32-null-val", STRING, ColumnSchema::NULLABLE), + ColumnSchema("str32-null ", STRING, ColumnSchema::NULLABLE), + }; CHECK_OK(base_.Reset(cols, 1)); base_ = SchemaBuilder(base_).Build(); // add IDs // Create an extended default schema - cols.emplace_back("int32-R ", INT32, false, false, false, kI32R, nullptr); - cols.emplace_back("int32-RW", INT32, false, false, false, kI32R, kI32W); - cols.emplace_back("str32-R ", STRING, false, false, false, kStrR, nullptr); - cols.emplace_back("str32-RW", STRING, false, false, false, kStrR, kStrW); + cols.emplace_back(ColumnSchemaBuilder() + .name("int32-R ") + .type(INT32) + .read_default(kI32R)); + cols.emplace_back(ColumnSchemaBuilder() + .name("int32-RW") + .type(INT32) + .read_default(kI32R) + .write_default(kI32W)); + cols.emplace_back(ColumnSchemaBuilder() + .name("str32-R ") + .type(STRING) + .read_default(kStrR)); + cols.emplace_back(ColumnSchemaBuilder() + .name("str32-RW") + .type(STRING) + .read_default(kStrR) + .write_default(kStrW)); CHECK_OK(defaults_.Reset(cols, 1)); defaults_ = SchemaBuilder(defaults_).Build(); // add IDs diff --git a/src/kudu/common/column_predicate-test.cc b/src/kudu/common/column_predicate-test.cc index dc7ca2ad1..bfebf71a1 100644 --- a/src/kudu/common/column_predicate-test.cc +++ b/src/kudu/common/column_predicate-test.cc @@ -959,16 +959,16 @@ class TestColumnPredicate : public KuduTest { }; TEST_F(TestColumnPredicate, TestMerge) { - TestMergeCombinations(ColumnSchema("c", INT8, true), + TestMergeCombinations(ColumnSchema("c", INT8, ColumnSchema::NULLABLE), vector<int8_t> { 0, 1, 2, 3, 4, 5, 6 }); - TestMergeCombinations(ColumnSchema("c", INT32, true), + TestMergeCombinations(ColumnSchema("c", INT32, ColumnSchema::NULLABLE), vector<int32_t> { -100, -10, -1, 0, 1, 10, 100 }); - TestMergeCombinations(ColumnSchema("c", STRING, true), + TestMergeCombinations(ColumnSchema("c", STRING, ColumnSchema::NULLABLE), vector<Slice> { "a", "b", "c", "d", "e", "f", "g" }); - TestMergeCombinations(ColumnSchema("c", BINARY, true), + TestMergeCombinations(ColumnSchema("c", BINARY, ColumnSchema::NULLABLE), vector<Slice> { Slice("", 0), Slice("\0", 1), Slice("\0\0", 2), @@ -1032,7 +1032,7 @@ TEST_F(TestColumnPredicate, TestInclusiveRange) { ASSERT_FALSE(ColumnPredicate::InclusiveRange(column, nullptr, &max, &arena)); } { - ColumnSchema column("c", INT32, true); + ColumnSchema column("c", INT32, ColumnSchema::NULLABLE); int32_t zero = 0; int32_t two = 2; int32_t three = 3; @@ -1312,13 +1312,13 @@ TEST_F(TestColumnPredicate, TestSelectivity) { Slice one_s("one", 3); int128_t one_dec = 1; - ColumnSchema column_i32("a", INT32, true); - ColumnSchema column_i64("b", INT64, true); - ColumnSchema column_d("c", DOUBLE, true); - ColumnSchema column_s("d", STRING, true); - ColumnSchema column_d32("e", DECIMAL32, true); - ColumnSchema column_d64("f", DECIMAL64, true); - ColumnSchema column_d128("g", DECIMAL128, true); + ColumnSchema column_i32("a", INT32, ColumnSchema::NULLABLE); + ColumnSchema column_i64("b", INT64, ColumnSchema::NULLABLE); + ColumnSchema column_d("c", DOUBLE, ColumnSchema::NULLABLE); + ColumnSchema column_s("d", STRING, ColumnSchema::NULLABLE); + ColumnSchema column_d32("e", DECIMAL32, ColumnSchema::NULLABLE); + ColumnSchema column_d64("f", DECIMAL64, ColumnSchema::NULLABLE); + ColumnSchema column_d128("g", DECIMAL128, ColumnSchema::NULLABLE); // Predicate type ASSERT_LT(SelectivityComparator(ColumnPredicate::IsNull(column_i32), @@ -1378,7 +1378,7 @@ TEST_F(TestColumnPredicate, TestSelectivity) { TEST_F(TestColumnPredicate, TestRedaction) { ASSERT_NE("", gflags::SetCommandLineOption("redact", "log")); - ColumnSchema column_i32("a", INT32, true); + ColumnSchema column_i32("a", INT32, ColumnSchema::NULLABLE); int32_t one_32 = 1; ASSERT_EQ("a = <redacted>", ColumnPredicate::Equality(column_i32, &one_32).ToString()); } @@ -1404,9 +1404,10 @@ TEST_F(TestColumnPredicate, TestBloomFilterMerge) { vector<uint64_t> values_int; FillBloomFilterAndValues(n_keys, &values_int, &b1, &b2); - TestMergeBloomFilterCombinations(ColumnSchema("c", INT64, true), {&b1}, values_int); - TestMergeBloomFilterCombinations(ColumnSchema("c", INT64, true), {&b1, &b2}, - values_int); + TestMergeBloomFilterCombinations( + ColumnSchema("c", INT64, ColumnSchema::NULLABLE), {&b1}, values_int); + TestMergeBloomFilterCombinations( + ColumnSchema("c", INT64, ColumnSchema::NULLABLE), {&b1, &b2}, values_int); // Test for STRING type. BlockBloomFilter b3(&allocator); @@ -1425,7 +1426,8 @@ TEST_F(TestColumnPredicate, TestBloomFilterMerge) { keys_slice.emplace_back(key_slice); } - TestMergeBloomFilterCombinations(ColumnSchema("c", STRING, true), {&b3}, keys_slice); + TestMergeBloomFilterCombinations( + ColumnSchema("c", STRING, ColumnSchema::NULLABLE), {&b3}, keys_slice); // Test for BINARY type BlockBloomFilter b4(&allocator); @@ -1444,27 +1446,27 @@ TEST_F(TestColumnPredicate, TestBloomFilterMerge) { b4.Insert(binary_keys[i]); } } - TestMergeBloomFilterCombinations(ColumnSchema("c", STRING, true), {&b4}, binary_keys); + TestMergeBloomFilterCombinations( + ColumnSchema("c", STRING, ColumnSchema::NULLABLE), {&b4}, binary_keys); } // Test ColumnPredicate operator (in-)equality. TEST_F(TestColumnPredicate, TestEquals) { - ColumnSchema c1("c1", INT32, true); + ColumnSchema c1("c1", INT32, ColumnSchema::NULLABLE); ASSERT_EQ(ColumnPredicate::None(c1), ColumnPredicate::None(c1)); - ColumnSchema c1a("c1", INT32, true); + ColumnSchema c1a("c1", INT32, ColumnSchema::NULLABLE); ASSERT_EQ(ColumnPredicate::None(c1), ColumnPredicate::None(c1a)); - ColumnSchema c2("c2", INT32, true); + ColumnSchema c2("c2", INT32, ColumnSchema::NULLABLE); ASSERT_NE(ColumnPredicate::None(c1), ColumnPredicate::None(c2)); - ColumnSchema c1string("c1", STRING, true); + ColumnSchema c1string("c1", STRING, ColumnSchema::NULLABLE); ASSERT_NE(ColumnPredicate::None(c1), ColumnPredicate::None(c1string)); const int kDefaultOf3 = 3; - ColumnSchema c1dflt("c1", INT32, /*is_nullable=*/false, - /*is_immutable=*/false, /*is_auto_incrementing=*/false, - /*read_default=*/&kDefaultOf3); + ColumnSchema c1dflt(ColumnSchemaBuilder() + .name("c1").type(INT32).read_default(&kDefaultOf3)); ASSERT_NE(ColumnPredicate::None(c1), ColumnPredicate::None(c1dflt)); } @@ -1474,8 +1476,8 @@ using TestColumnPredicateDeathTest = TestColumnPredicate; // have the same column name and type as 'this'. TEST_F(TestColumnPredicateDeathTest, TestMergeRequiresNameAndType) { - ColumnSchema c1int32("c1", INT32, true); - ColumnSchema c2int32("c2", INT32, true); + ColumnSchema c1int32("c1", INT32, ColumnSchema::NULLABLE); + ColumnSchema c2int32("c2", INT32, ColumnSchema::NULLABLE); vector<int32_t> values = { 0, 1, 2, 3 }; EXPECT_DEATH({ @@ -1486,7 +1488,7 @@ TEST_F(TestColumnPredicateDeathTest, TestMergeRequiresNameAndType) { PredicateType::None); }, "COMPARE_NAME_AND_TYPE"); - ColumnSchema c1int16("c1", INT16, true); + ColumnSchema c1int16("c1", INT16, ColumnSchema::NULLABLE); EXPECT_DEATH({ // This should crash because the columns have different types. TestMerge(ColumnPredicate::Equality(c1int32, &values[0]), @@ -1506,7 +1508,8 @@ class ColumnPredicateBenchmark : public KuduTest { const int num_iters = AllowSlowTests() ? 1000000 : 100; const int num_evals = num_iters * kNumRows; - for (bool nullable : {false, true}) { + for (ColumnSchema::Nullability nullable : {ColumnSchema::NOT_NULL, + ColumnSchema::NULLABLE}) { ColumnSchema cs("c", kColType, nullable); auto pred = pred_factory(cs); if (pred.predicate_type() == PredicateType::None) { @@ -1518,7 +1521,7 @@ class ColumnPredicateBenchmark : public KuduTest { ScopedColumnBlock<kColType> b(kNumRows); for (int i = 0; i < kNumRows; i++) { b[i] = rand() % 3; - if (nullable) { + if (nullable == ColumnSchema::NULLABLE) { b.SetCellIsNull(i, rand() % 10 == 1); } } diff --git a/src/kudu/common/generic_iterators-test.cc b/src/kudu/common/generic_iterators-test.cc index 9864f892f..f2353c655 100644 --- a/src/kudu/common/generic_iterators-test.cc +++ b/src/kudu/common/generic_iterators-test.cc @@ -85,11 +85,11 @@ static const Schema kIntSchema({ ColumnSchema("val", INT64) }, /*key_columns=*/1); static const bool kIsDeletedReadDefault = false; static const Schema kIntSchemaWithVCol({ ColumnSchema("val", INT64), - ColumnSchema("is_deleted", IS_DELETED, - /*is_nullable=*/false, - /*is_immutable=*/false, - /*is_auto_incrementing=*/false, - /*read_default=*/&kIsDeletedReadDefault) }, + ColumnSchemaBuilder() + .name("is_deleted") + .type(IS_DELETED) + .read_default(&kIsDeletedReadDefault) + }, /*key_columns=*/1); // Test iterator which just yields integer rows from a provided diff --git a/src/kudu/common/key_util-test.cc b/src/kudu/common/key_util-test.cc index 29ded24ec..639f3889c 100644 --- a/src/kudu/common/key_util-test.cc +++ b/src/kudu/common/key_util-test.cc @@ -52,7 +52,7 @@ class KeyUtilTest : public KuduTest { TEST_F(KeyUtilTest, TestIncrementNonCompositePrimaryKey) { Schema schema({ ColumnSchema("key", INT32), ColumnSchema("other_col", INT32), - ColumnSchema("other_col2", STRING, true) }, + ColumnSchema("other_col2", STRING, ColumnSchema::NULLABLE) }, 1); KuduPartialRow p_row(&schema); ContiguousRow row(&schema, row_data(&p_row)); @@ -71,7 +71,7 @@ TEST_F(KeyUtilTest, TestIncrementNonCompositePrimaryKey) { TEST_F(KeyUtilTest, TestIncrementInt128PrimaryKey) { Schema schema({ ColumnSchema("key", INT128), ColumnSchema("other_col", INT32), - ColumnSchema("other_col2", STRING, true) }, + ColumnSchema("other_col2", STRING, ColumnSchema::NULLABLE) }, 1); KuduPartialRow p_row(&schema); ContiguousRow row(&schema, row_data(&p_row)); @@ -90,7 +90,7 @@ TEST_F(KeyUtilTest, TestIncrementInt128PrimaryKey) { TEST_F(KeyUtilTest, TestIncrementCompositePrimaryKey) { Schema schema({ ColumnSchema("k1", INT32), ColumnSchema("k2", INT32), - ColumnSchema("other_col", STRING, true) }, + ColumnSchema("other_col", STRING, ColumnSchema::NULLABLE) }, 2); KuduPartialRow p_row(&schema); @@ -118,7 +118,7 @@ TEST_F(KeyUtilTest, TestIncrementCompositePrimaryKey) { TEST_F(KeyUtilTest, TestIncrementCompositeIntStringPrimaryKey) { Schema schema({ ColumnSchema("k1", INT32), ColumnSchema("k2", STRING), - ColumnSchema("other_col", STRING, true) }, + ColumnSchema("other_col", STRING, ColumnSchema::NULLABLE) }, 2); KuduPartialRow p_row(&schema); @@ -139,7 +139,7 @@ TEST_F(KeyUtilTest, TestIncrementCompositeIntStringPrimaryKey) { TEST_F(KeyUtilTest, TestIncrementCompositeStringIntPrimaryKey) { Schema schema({ ColumnSchema("k1", STRING), ColumnSchema("k2", INT32), - ColumnSchema("other_col", STRING, true) }, + ColumnSchema("other_col", STRING, ColumnSchema::NULLABLE) }, 2); KuduPartialRow p_row(&schema); diff --git a/src/kudu/common/partial_row-test.cc b/src/kudu/common/partial_row-test.cc index ccb9e181b..34387eadc 100644 --- a/src/kudu/common/partial_row-test.cc +++ b/src/kudu/common/partial_row-test.cc @@ -41,12 +41,18 @@ class PartialRowTest : public KuduTest { : schema_({ ColumnSchema("key", INT32), ColumnSchema("int_val", INT32), ColumnSchema("uint64_val", UINT64), - ColumnSchema("string_val", STRING, true), - ColumnSchema("binary_val", BINARY, true), - ColumnSchema("decimal_val", DECIMAL32, true, false, false, nullptr, nullptr, - ColumnStorageAttributes(), ColumnTypeAttributes(6, 2)), - ColumnSchema("varchar_val", VARCHAR, true, false, false, nullptr, nullptr, - ColumnStorageAttributes(), ColumnTypeAttributes(10)) }, + ColumnSchema("string_val", STRING, ColumnSchema::NULLABLE), + ColumnSchema("binary_val", BINARY, ColumnSchema::NULLABLE), + ColumnSchemaBuilder() + .name("decimal_val") + .type(DECIMAL32) + .nullable(true) + .type_attributes({6, 2}), + ColumnSchemaBuilder() + .name("varchar_val") + .type(VARCHAR) + .nullable(true) + .type_attributes(ColumnTypeAttributes(10)) }, 1) { SeedRandom(); } diff --git a/src/kudu/common/partition-test.cc b/src/kudu/common/partition-test.cc index 2de88ecea..9baf53fc5 100644 --- a/src/kudu/common/partition-test.cc +++ b/src/kudu/common/partition-test.cc @@ -876,10 +876,14 @@ TEST_F(PartitionTest, TestIncrementRangePartitionStringBounds) { } TEST_F(PartitionTest, TestVarcharRangePartitions) { - Schema schema({ ColumnSchema("c1", VARCHAR, false, false, false, nullptr, nullptr, - ColumnStorageAttributes(), ColumnTypeAttributes(10)), - ColumnSchema("c2", VARCHAR, false, false, false, nullptr, nullptr, - ColumnStorageAttributes(), ColumnTypeAttributes(10)) }, + Schema schema({ ColumnSchemaBuilder() + .name("c1") + .type(VARCHAR) + .type_attributes(ColumnTypeAttributes(10)), + ColumnSchemaBuilder() + .name("c2") + .type(VARCHAR) + .type_attributes(ColumnTypeAttributes(10)) }, { ColumnId(0), ColumnId(1) }, 2); PartitionSchemaPB schema_builder; @@ -2320,7 +2324,9 @@ TEST_F(PartitionTest, PartitionKeyWithAutoIncrementingColumn) { // NON-UNIQUE PRIMARY KEY (c1) // PARTITION BY HASH (c1) PARTITIONS 2; Schema schema({ ColumnSchema("c1", STRING), - ColumnSchema("auto_increment_id", INT64, false, false, true), + ColumnSchemaBuilder() + .name("auto_increment_id") + .auto_incrementing(true), ColumnSchema("c2", STRING) }, { ColumnId(0), ColumnId(1), ColumnId(2) }, 2); diff --git a/src/kudu/common/row_operations-test.cc b/src/kudu/common/row_operations-test.cc index d6d5b1655..a81f6d9f1 100644 --- a/src/kudu/common/row_operations-test.cc +++ b/src/kudu/common/row_operations-test.cc @@ -232,7 +232,8 @@ void AddFuzzedColumn(SchemaBuilder* builder, t = rand_types[random() % arraysize(rand_types)]; } bool nullable = random() & 1; - CHECK_OK(builder->AddColumn(name, t, nullable, NULL, NULL)); + CHECK_OK(builder->AddColumn( + ColumnSchemaBuilder().name(name).type(t).nullable(nullable))); } // Generate a randomized schema, where some columns might be missing, @@ -336,12 +337,12 @@ TEST_F(RowOperationsTest, SchemaFuzz) { // One case from SchemaFuzz which failed previously. TEST_F(RowOperationsTest, TestFuzz1) { SchemaBuilder client_schema_builder; - ASSERT_OK(client_schema_builder.AddColumn("c1", INT32, false, nullptr, nullptr)); - ASSERT_OK(client_schema_builder.AddColumn("c2", STRING, false, nullptr, nullptr)); + ASSERT_OK(client_schema_builder.AddColumn("c1", INT32)); + ASSERT_OK(client_schema_builder.AddColumn("c2", STRING)); Schema client_schema = client_schema_builder.BuildWithoutIds(); SchemaBuilder server_schema_builder; - ASSERT_OK(server_schema_builder.AddColumn("c1", INT32, false, nullptr, nullptr)); - ASSERT_OK(server_schema_builder.AddColumn("c2", STRING, false, nullptr, nullptr)); + ASSERT_OK(server_schema_builder.AddColumn("c1", INT32)); + ASSERT_OK(server_schema_builder.AddColumn("c2", STRING)); Schema server_schema = server_schema_builder.Build(); KuduPartialRow row(&client_schema); ASSERT_OK(row.SetInt32(0, 12345)); @@ -352,12 +353,12 @@ TEST_F(RowOperationsTest, TestFuzz1) { // Another case from SchemaFuzz which failed previously. TEST_F(RowOperationsTest, TestFuzz2) { SchemaBuilder client_schema_builder; - ASSERT_OK(client_schema_builder.AddColumn("c1", STRING, true, nullptr, nullptr)); - ASSERT_OK(client_schema_builder.AddColumn("c2", STRING, false, nullptr, nullptr)); + ASSERT_OK(client_schema_builder.AddNullableColumn("c1", STRING)); + ASSERT_OK(client_schema_builder.AddColumn("c2", STRING)); Schema client_schema = client_schema_builder.BuildWithoutIds(); SchemaBuilder server_schema_builder; - ASSERT_OK(server_schema_builder.AddColumn("c1", STRING, true, nullptr, nullptr)); - ASSERT_OK(server_schema_builder.AddColumn("c2", STRING, false, nullptr, nullptr)); + ASSERT_OK(server_schema_builder.AddNullableColumn("c1", STRING)); + ASSERT_OK(server_schema_builder.AddColumn("c2", STRING)); Schema server_schema = server_schema_builder.Build(); KuduPartialRow row(&client_schema); ASSERT_OK(row.SetNull(0)); @@ -396,7 +397,7 @@ string TestProjection(RowOperationsPB::Type type, TEST_F(RowOperationsTest, ProjectionTestWholeSchemaSpecified) { Schema client_schema({ ColumnSchema("key", INT32), ColumnSchema("int_val", INT32), - ColumnSchema("string_val", STRING, true) }, + ColumnSchema("string_val", STRING, ColumnSchema::NULLABLE) }, 1); // Test a row missing 'key', which is key column. { @@ -462,17 +463,24 @@ TEST_F(RowOperationsTest, ProjectionTestWithDefaults) { int32_t non_null_default = 456; SchemaBuilder b; CHECK_OK(b.AddKeyColumn("key", INT32)); - CHECK_OK(b.AddColumn("nullable_with_default", INT32, true, - &nullable_default, &nullable_default)); - CHECK_OK(b.AddColumn("non_null_with_default", INT32, false, - &non_null_default, &non_null_default)); + CHECK_OK(b.AddColumn(ColumnSchemaBuilder() + .name("nullable_with_default") + .type(INT32) + .nullable(true) + .read_default(&nullable_default) + .write_default(&nullable_default))); + CHECK_OK(b.AddColumn(ColumnSchemaBuilder() + .name("non_null_with_default") + .type(INT32) + .read_default(&non_null_default) + .write_default(&non_null_default))); Schema server_schema = b.Build(); // Clients may not have the defaults specified. // TODO: evaluate whether this should be true - how "dumb" should clients be? Schema client_schema({ ColumnSchema("key", INT32), - ColumnSchema("nullable_with_default", INT32, true), - ColumnSchema("non_null_with_default", INT32, false) }, + ColumnSchema("nullable_with_default", INT32, ColumnSchema::NULLABLE), + ColumnSchema("non_null_with_default", INT32) }, 1); // Specify just the key. The other two columns have defaults, so they'll get filled in. @@ -529,8 +537,11 @@ TEST_F(RowOperationsTest, ProjectionTestWithClientHavingValidSubset) { SchemaBuilder b; CHECK_OK(b.AddKeyColumn("key", INT32)); CHECK_OK(b.AddColumn("int_val", INT32)); - CHECK_OK(b.AddColumn("new_int_with_default", INT32, false, - &nullable_default, &nullable_default)); + CHECK_OK(b.AddColumn(ColumnSchemaBuilder() + .name("new_int_with_default") + .type(INT32) + .read_default(&nullable_default) + .write_default(&nullable_default))); CHECK_OK(b.AddNullableColumn("new_nullable_int", INT32)); Schema server_schema = b.Build(); @@ -583,7 +594,7 @@ TEST_F(RowOperationsTest, ProjectionTestWithClientHavingInvalidSubset) { TEST_F(RowOperationsTest, TestProjectUpdates) { Schema client_schema({ ColumnSchema("key", INT32), ColumnSchema("int_val", INT32), - ColumnSchema("string_val", STRING, true) }, + ColumnSchema("string_val", STRING, ColumnSchema::NULLABLE) }, 1); Schema server_schema = SchemaBuilder(client_schema).Build(); @@ -644,12 +655,12 @@ TEST_F(RowOperationsTest, TestProjectUpdates) { // sure the name-based projection is functioning. TEST_F(RowOperationsTest, TestProjectUpdatesReorderedColumns) { Schema client_schema({ ColumnSchema("key", INT32), - ColumnSchema("string_val", STRING, true), + ColumnSchema("string_val", STRING, ColumnSchema::NULLABLE), ColumnSchema("int_val", INT32) }, 1); Schema server_schema({ ColumnSchema("key", INT32), ColumnSchema("int_val", INT32), - ColumnSchema("string_val", STRING, true) }, + ColumnSchema("string_val", STRING, ColumnSchema::NULLABLE) }, 1); server_schema = SchemaBuilder(server_schema).Build(); @@ -664,11 +675,11 @@ TEST_F(RowOperationsTest, TestProjectUpdatesReorderedColumns) { // This is OK on an update. TEST_F(RowOperationsTest, DISABLED_TestProjectUpdatesSubsetOfColumns) { Schema client_schema({ ColumnSchema("key", INT32), - ColumnSchema("string_val", STRING, true) }, + ColumnSchema("string_val", STRING, ColumnSchema::NULLABLE) }, 1); Schema server_schema({ ColumnSchema("key", INT32), ColumnSchema("int_val", INT32), - ColumnSchema("string_val", STRING, true) }, + ColumnSchema("string_val", STRING, ColumnSchema::NULLABLE) }, 1); server_schema = SchemaBuilder(server_schema).Build(); @@ -700,7 +711,7 @@ TEST_F(RowOperationsTest, TestProjectDeletes) { Schema client_schema({ ColumnSchema("key", INT32), ColumnSchema("key_2", INT32), ColumnSchema("int_val", INT32), - ColumnSchema("string_val", STRING, true) }, + ColumnSchema("string_val", STRING, ColumnSchema::NULLABLE) }, 2); Schema server_schema = SchemaBuilder(client_schema).Build(); diff --git a/src/kudu/common/scan_spec-test.cc b/src/kudu/common/scan_spec-test.cc index 90549a32a..67da11e9e 100644 --- a/src/kudu/common/scan_spec-test.cc +++ b/src/kudu/common/scan_spec-test.cc @@ -21,6 +21,7 @@ #include <cstring> #include <optional> #include <string> +#include <tuple> #include <unordered_map> #include <utility> #include <vector> @@ -227,7 +228,7 @@ class CompositeIntKeysTest : public TestScanSpec { Schema({ ColumnSchema("a", INT8), ColumnSchema("b", INT8), ColumnSchema("c", INT8), - ColumnSchema("d", INT8, true), + ColumnSchema("d", INT8, ColumnSchema::NULLABLE), ColumnSchema("e", INT8) }, 3)) { } @@ -1380,7 +1381,7 @@ TEST_F(CompositeIntKeysTest, TestGetMissingColumns) { } { // Projection: d e. - Schema projection({ ColumnSchema("d", INT8, true), + Schema projection({ ColumnSchema("d", INT8, ColumnSchema::NULLABLE), ColumnSchema("e", INT8) }, 0); vector<ColumnSchema> missing_cols = spec.GetMissingColumns(projection); EXPECT_EQ(1, missing_cols.size()); @@ -1391,7 +1392,7 @@ TEST_F(CompositeIntKeysTest, TestGetMissingColumns) { { // Projection: b d e. Schema projection({ ColumnSchema("b", INT8), - ColumnSchema("d", INT8, true), + ColumnSchema("d", INT8, ColumnSchema::NULLABLE), ColumnSchema("e", INT8) }, 0); vector<ColumnSchema> missing_cols = spec.GetMissingColumns(projection); EXPECT_EQ(0, missing_cols.size()); diff --git a/src/kudu/common/schema-test.cc b/src/kudu/common/schema-test.cc index d2a6d1a7e..787425562 100644 --- a/src/kudu/common/schema-test.cc +++ b/src/kudu/common/schema-test.cc @@ -108,7 +108,7 @@ TEST_F(TestSchema, TestSchema) { ASSERT_GT(empty_schema.memory_footprint_excluding_this(), 0); ColumnSchema col1("key", STRING); - ColumnSchema col2("uint32val", UINT32, true); + ColumnSchema col2("uint32val", UINT32, ColumnSchema::NULLABLE); ColumnSchema col3("int32val", INT32); vector<ColumnSchema> cols = { col1, col2, col3 }; @@ -185,7 +185,7 @@ TEST_P(ParameterizedSchemaTest, TestCopyAndMove) { }; ColumnSchema col1("key", STRING); - ColumnSchema col2("uint32val", UINT32, true); + ColumnSchema col2("uint32val", UINT32, ColumnSchema::NULLABLE); ColumnSchema col3("int32val", INT32); vector<ColumnSchema> cols = { col1, col2, col3 }; @@ -232,15 +232,20 @@ TEST_P(ParameterizedSchemaTest, TestCopyAndMove) { // Test basic functionality of Schema definition with decimal columns TEST_F(TestSchema, TestSchemaWithDecimal) { ColumnSchema col1("key", STRING); - ColumnSchema col2("decimal32val", DECIMAL32, false, false, false, - nullptr, nullptr, ColumnStorageAttributes(), - ColumnTypeAttributes(9, 4)); - ColumnSchema col3("decimal64val", DECIMAL64, true, false, false, - nullptr, nullptr, ColumnStorageAttributes(), - ColumnTypeAttributes(18, 10)); - ColumnSchema col4("decimal128val", DECIMAL128, true, false, false, - nullptr, nullptr, ColumnStorageAttributes(), - ColumnTypeAttributes(38, 2)); + ColumnSchema col2(ColumnSchemaBuilder() + .name("decimal32val") + .type(DECIMAL32) + .type_attributes(ColumnTypeAttributes(9, 4))); + ColumnSchema col3(ColumnSchemaBuilder() + .name("decimal64val") + .type(DECIMAL64) + .type_attributes(ColumnTypeAttributes(18, 10)) + .nullable(true)); + ColumnSchema col4(ColumnSchemaBuilder() + .name("decimal128val") + .type(DECIMAL128) + .type_attributes(ColumnTypeAttributes(38, 2)) + .nullable(true)); vector<ColumnSchema> cols = { col1, col2, col3, col4 }; Schema schema(cols, 1); @@ -266,18 +271,26 @@ TEST_F(TestSchema, TestSchemaWithDecimal) { // Test Schema::Equals respects decimal column attributes TEST_F(TestSchema, TestSchemaEqualsWithDecimal) { ColumnSchema col1("key", STRING); - ColumnSchema col_18_10("decimal64val", DECIMAL64, true, false, false, - nullptr, nullptr, ColumnStorageAttributes(), - ColumnTypeAttributes(18, 10)); - ColumnSchema col_18_9("decimal64val", DECIMAL64, true, false,false, - nullptr, nullptr, ColumnStorageAttributes(), - ColumnTypeAttributes(18, 9)); - ColumnSchema col_17_10("decimal64val", DECIMAL64, true, false, false, - nullptr, nullptr, ColumnStorageAttributes(), - ColumnTypeAttributes(17, 10)); - ColumnSchema col_17_9("decimal64val", DECIMAL64, true, false, false, - nullptr, nullptr, ColumnStorageAttributes(), - ColumnTypeAttributes(17, 9)); + ColumnSchema col_18_10(ColumnSchemaBuilder() + .name("decimal64val") + .type(DECIMAL64) + .nullable(true) + .type_attributes({18, 10})); + ColumnSchema col_18_9(ColumnSchemaBuilder() + .name("decimal64val") + .type(DECIMAL64) + .type_attributes({18, 9}) + .nullable(true)); + ColumnSchema col_17_10(ColumnSchemaBuilder() + .name("decimal64val") + .type(DECIMAL64) + .type_attributes({17, 10}) + .nullable(true)); + ColumnSchema col_17_9(ColumnSchemaBuilder() + .name("decimal64val") + .type(DECIMAL64) + .type_attributes({17, 9}) + .nullable(true)); Schema schema_18_10({ col1, col_18_10 }, 1); Schema schema_18_9({ col1, col_18_9 }, 1); @@ -294,8 +307,13 @@ TEST_F(TestSchema, TestColumnSchemaEquals) { Slice default_str("read-write default"); ColumnSchema col1("key", STRING); ColumnSchema col2("key1", STRING); - ColumnSchema col3("key", STRING, true); - ColumnSchema col4("key", STRING, true, false, false, &default_str, &default_str); + ColumnSchema col3("key", STRING, ColumnSchema::NULLABLE); + ColumnSchema col4(ColumnSchemaBuilder() + .name("key") + .type(STRING) + .nullable(true) + .read_default(&default_str) + .write_default(&default_str)); ASSERT_TRUE(col1.Equals(col1)); ASSERT_FALSE(col1.Equals(col2, ColumnSchema::COMPARE_NAME)); @@ -318,11 +336,11 @@ TEST_F(TestSchema, TestSchemaEquals) { 2); Schema schema3({ ColumnSchema("col1", STRING), ColumnSchema("col2", UINT32), - ColumnSchema("col3", UINT32, true) }, + ColumnSchema("col3", UINT32, ColumnSchema::NULLABLE) }, 2); Schema schema4({ ColumnSchema("col1", STRING), ColumnSchema("col2", UINT32), - ColumnSchema("col3", UINT32, false) }, + ColumnSchema("col3", UINT32) }, 2); ASSERT_NE(schema1, schema2); ASSERT_TRUE(schema1.KeyEquals(schema1)); @@ -417,12 +435,21 @@ TEST_F(TestSchema, TestProjectTypeMismatch) { // Test projection when the some columns in the projection // are not present in the base schema TEST_F(TestSchema, TestProjectMissingColumn) { - Schema schema1({ ColumnSchema("key", STRING), ColumnSchema("val", UINT32) }, 1); - Schema schema2({ ColumnSchema("val", UINT32), ColumnSchema("non_present", STRING) }, 0); - Schema schema3({ ColumnSchema("val", UINT32), ColumnSchema("non_present", UINT32, true) }, 0); + Schema schema1({ ColumnSchema("key", STRING), + ColumnSchema("val", UINT32) }, + 1); + Schema schema2({ ColumnSchema("val", UINT32), + ColumnSchema("non_present", STRING) }, + 0); + Schema schema3({ ColumnSchema("val", UINT32), + ColumnSchema("non_present", UINT32, ColumnSchema::NULLABLE) }, + 0); uint32_t default_value = 15; Schema schema4({ ColumnSchema("val", UINT32), - ColumnSchema("non_present", UINT32, false, false, false, &default_value) }, + ColumnSchemaBuilder() + .name("non_present") + .type(UINT32) + .read_default(&default_value) }, 0); RowProjector row_projector(&schema1, &schema2); @@ -491,11 +518,10 @@ TEST_F(TestSchema, TestGetMappedReadProjection) { 1); const bool kReadDefault = false; Schema projection({ ColumnSchema("key", STRING), - ColumnSchema("deleted", IS_DELETED, - /*is_nullable=*/false, - /*is_immutable=*/false, - /*is_auto_incrementing=*/false, - /*read_default=*/&kReadDefault) }, + ColumnSchemaBuilder() + .name("deleted") + .type(IS_DELETED) + .read_default(&kReadDefault) }, 1); Schema mapped; @@ -521,22 +547,18 @@ TEST_F(TestSchema, TestGetMappedReadProjection) { // defaults are rejected. Schema nullable_projection; Status s = nullable_projection.Reset({ ColumnSchema("key", STRING), - ColumnSchema("deleted", IS_DELETED, - /*is_nullable=*/true, - /*is_immutable=*/false, - /*is_auto_incrementing=*/false, - /*read_default=*/&kReadDefault) }, + ColumnSchemaBuilder() + .name("deleted") + .type(IS_DELETED) + .nullable(true) + .read_default(&kReadDefault) }, 1); ASSERT_FALSE(s.ok()); ASSERT_STR_CONTAINS(s.ToString(), "must not be nullable"); Schema no_default_projection; s = no_default_projection.Reset({ ColumnSchema("key", STRING), - ColumnSchema("deleted", IS_DELETED, - /*is_nullable=*/false, - /*is_immutable=*/false, - /*is_auto_incrementing*/false, - /*read_default=*/nullptr) }, + ColumnSchema("deleted", IS_DELETED) }, 1); ASSERT_FALSE(s.ok()); ASSERT_STR_CONTAINS(s.ToString(), "must have a default value for read"); diff --git a/src/kudu/common/schema.cc b/src/kudu/common/schema.cc index 7a122cada..9c4132f40 100644 --- a/src/kudu/common/schema.cc +++ b/src/kudu/common/schema.cc @@ -142,13 +142,13 @@ Status ColumnSchema::ApplyDelta(const ColumnSchemaDelta& col_delta) { } if (col_delta.encoding) { - attributes_.encoding = *col_delta.encoding; + storage_attributes_.encoding = *col_delta.encoding; } if (col_delta.compression) { - attributes_.compression = *col_delta.compression; + storage_attributes_.compression = *col_delta.compression; } if (col_delta.cfile_block_size) { - attributes_.cfile_block_size = *col_delta.cfile_block_size; + storage_attributes_.cfile_block_size = *col_delta.cfile_block_size; } if (col_delta.new_comment) { comment_ = col_delta.new_comment.value(); @@ -181,7 +181,7 @@ string ColumnSchema::TypeToString() const { string ColumnSchema::AttrToString() const { return Substitute("$0 $1 $2", - attributes_.ToString(), + storage_attributes_.ToString(), has_read_default() ? Stringify(read_default_value()) : "-", has_write_default() ? Stringify(write_default_value()) : "-"); } @@ -615,48 +615,21 @@ void SchemaBuilder::Reset(const Schema& schema) { } } -Status SchemaBuilder::AddKeyColumn(const string& name, DataType type) { - return AddColumn(ColumnSchema(name, type), true); -} - -Status SchemaBuilder::AddColumn(const string& name, - DataType type, - bool is_nullable, - const void* read_default, - const void* write_default) { - return AddColumn(name, type, is_nullable, false, read_default, write_default); -} - -Status SchemaBuilder::AddColumn(const std::string& name, - DataType type, - bool is_nullable, - bool is_immutable, - const void* read_default, - const void* write_default) { - return AddColumn(name, type, is_nullable, is_immutable, false, read_default, write_default); -} - -Status SchemaBuilder::AddColumn(const std::string& name, - DataType type, - bool is_nullable, - bool is_immutable, - bool is_auto_incrementing, - const void* read_default, - const void* write_default) { - if (name.empty()) { - return Status::InvalidArgument("column name must be non-empty"); +Status SchemaBuilder::AddColumn(const ColumnSchema& column, bool is_key) { + if (!InsertIfNotPresent(&col_names_, column.name())) { + return Status::AlreadyPresent("The column already exists", column.name()); } - - if (is_auto_incrementing) { - DCHECK_EQ(type, INT64); - DCHECK(!is_nullable); - DCHECK(!is_immutable); - DCHECK_EQ(read_default, (void *)nullptr); - DCHECK_EQ(write_default, (void *)nullptr); + if (is_key) { + cols_.insert(cols_.begin() + num_key_columns_, column); + col_ids_.insert(col_ids_.begin() + num_key_columns_, next_id_); + ++num_key_columns_; + } else { + cols_.push_back(column); + col_ids_.push_back(next_id_); } - return AddColumn(ColumnSchema(name, type, is_nullable, is_immutable, - is_auto_incrementing, read_default, write_default), false); + next_id_ = ColumnId(next_id_ + 1); + return Status::OK(); } Status SchemaBuilder::RemoveColumn(const string& name) { @@ -710,25 +683,6 @@ Status SchemaBuilder::RenameColumn(const string& old_name, const string& new_nam return Status::IllegalState("Unable to rename existing column"); } -Status SchemaBuilder::AddColumn(const ColumnSchema& column, bool is_key) { - if (ContainsKey(col_names_, column.name())) { - return Status::AlreadyPresent("The column already exists", column.name()); - } - - col_names_.insert(column.name()); - if (is_key) { - cols_.insert(cols_.begin() + num_key_columns_, column); - col_ids_.insert(col_ids_.begin() + num_key_columns_, next_id_); - ++num_key_columns_; - } else { - cols_.push_back(column); - col_ids_.push_back(next_id_); - } - - next_id_ = ColumnId(next_id_ + 1); - return Status::OK(); -} - Status SchemaBuilder::ApplyColumnSchemaDelta(const ColumnSchemaDelta& col_delta) { // if the column will be renamed, check if 'new_name' is already in use if (col_delta.new_name && ContainsKey(col_names_, *col_delta.new_name)) { diff --git a/src/kudu/common/schema.h b/src/kudu/common/schema.h index 7f06d70a0..692b88ee6 100644 --- a/src/kudu/common/schema.h +++ b/src/kudu/common/schema.h @@ -204,57 +204,47 @@ struct ColumnSchemaDelta { // // Holds the data type as well as information about nullability & column name. // In the future, it may hold information about annotations, etc. -class ColumnSchema { +class ColumnSchema final { public: + // Enumeration to express a column's nullability. + enum Nullability { + NOT_NULL, + NULLABLE, + }; + + // A minimalistic constructor with only name, type, and nullability of + // the column to specify: the latter parameter is optional and by default + // is set to Nullability::NOT_NULL. + // // name: column name // type: column type (e.g. UINT8, INT32, STRING, ...) - // is_nullable: true if a row value can be null - // is_immutable: true if the column is immutable. - // Immutable column means the cell value can not be updated after the first insert. - // is_auto_incrementing: true if the column is auto-incrementing column. - // Auto-incrementing column cannot be updated but written to by a client and is auto - // filled in by Kudu by incrementing the previous highest written value to the tablet. - // There can only be a single auto-incrementing column per table. - // read_default: default value used on read if the column was not present before alter. - // The value will be copied and released on ColumnSchema destruction. - // write_default: default value added to the row if the column value was - // not specified on insert. - // The value will be copied and released on ColumnSchema destruction. - // comment: the comment for the column. + // nullability: NOT_NULL or NULLABLE // // Example: - // ColumnSchema col_a("a", UINT32) - // ColumnSchema col_b("b", STRING, true); - // ColumnSchema col_b("b", STRING, false, true); + // ColumnSchema col_a("a", UINT32); + // ColumnSchema col_b("b", STRING, ColumnSchema::NULLABLE); + // + // Don't use this constructor if there are more column parameters to specify. + // Instead, switch to ColumnSchemaBuilder(): // uint32_t default_i32 = -15; - // ColumnSchema col_c("c", INT32, false, false, false, &default_i32); + // ColumnSchema col_c(ColumnSchemaBuilder() + // .name("c") + // .type(INT32) + // .read_default(&default_i32)); + // // Slice default_str("Hello"); - // ColumnSchema col_d("d", STRING, false, false, false, &default_str); - // ColumnSchema col_e("e", STRING, false, false, true, &default_str); + // ColumnSchema col_d(ColumnSchemaBuilder() + // .name("d") + // .type(STRING) + // .nullable(true) + // .read_default(&default_str) + // .write_default(&default_str)); ColumnSchema(std::string name, DataType type, - bool is_nullable = false, - bool is_immutable = false, - bool is_auto_incrementing = false, - const void* read_default = nullptr, - const void* write_default = nullptr, - ColumnStorageAttributes attributes = {}, - ColumnTypeAttributes type_attributes = {}, - std::string comment = "") + Nullability nullability = NOT_NULL) : name_(std::move(name)), type_info_(GetTypeInfo(type)), - is_nullable_(is_nullable), - is_immutable_(is_immutable), - is_auto_incrementing_(is_auto_incrementing), - read_default_(read_default ? std::make_shared<Variant>(type, read_default) : nullptr), - attributes_(attributes), - type_attributes_(type_attributes), - comment_(std::move(comment)) { - if (write_default == read_default) { - write_default_ = read_default_; - } else if (write_default != nullptr) { - write_default_ = std::make_shared<Variant>(type, write_default); - } + is_nullable_(nullability != NOT_NULL) { } const TypeInfo* type_info() const { @@ -415,7 +405,7 @@ class ColumnSchema { // struct is so that in the future, they may be moved out to a more // appropriate location as opposed to parts of ColumnSchema. const ColumnStorageAttributes& attributes() const { - return attributes_; + return storage_attributes_; } const ColumnTypeAttributes& type_attributes() const { @@ -458,23 +448,233 @@ class ColumnSchema { size_t memory_footprint_including_this() const; private: + friend class ColumnSchemaBuilder; friend class SchemaBuilder; + // Constructor with initializers for all the fields. + // + // name: column name + // type: column type (e.g. UINT8, INT32, STRING, ...) + // is_nullable: true if a row value can be null + // is_immutable: true if the column is immutable. + // Immutable column means the cell value can not be updated after the first insert. + // is_auto_incrementing: true if the column is auto-incrementing column. + // Auto-incrementing column cannot be updated but written to by a client and is auto + // filled in by Kudu by incrementing the previous highest written value to the tablet. + // There can only be a single auto-incrementing column per table. + // read_default: default value used on read if the column was not present before alter. + // The value will be copied and released on ColumnSchema destruction. + // write_default: default value added to the row if the column value was + // not specified on insert. + // The value will be copied and released on ColumnSchema destruction + // storage_attributes: CFile-level attributes for corresponding column blocks + // type_attributes: attributes of the column type (e.g. precision for DECIMAL) + // comment: the comment for the column. + // + // This constructor is private to encourage using ColumnSchemaBuilder when + // it's necessary to specify more attributes than just name, type, and column + // nullability. + ColumnSchema(std::string name, + DataType type, + bool is_nullable, + bool is_immutable, + bool is_auto_incrementing, + std::shared_ptr<Variant> read_default, + std::shared_ptr<Variant> write_default, + ColumnStorageAttributes storage_attributes, + ColumnTypeAttributes type_attributes, + std::string comment) + : name_(std::move(name)), + type_info_(GetTypeInfo(type)), + is_nullable_(is_nullable), + is_immutable_(is_immutable), + is_auto_incrementing_(is_auto_incrementing), + read_default_(std::move(read_default)), + write_default_(std::move(write_default)), + storage_attributes_(storage_attributes), + type_attributes_(type_attributes), + comment_(std::move(comment)) { + } + void set_name(const std::string& name) { name_ = name; } - std::string name_; - const TypeInfo* type_info_; - bool is_nullable_; - bool is_immutable_; - bool is_auto_incrementing_; + std::string name_{}; + const TypeInfo* type_info_ = nullptr; + bool is_nullable_ = false; + bool is_immutable_ = false; + bool is_auto_incrementing_ = false; // use shared_ptr since the ColumnSchema is always copied around. - std::shared_ptr<Variant> read_default_; - std::shared_ptr<Variant> write_default_; - ColumnStorageAttributes attributes_; - ColumnTypeAttributes type_attributes_; - std::string comment_; + std::shared_ptr<Variant> read_default_{}; + std::shared_ptr<Variant> write_default_{}; + ColumnStorageAttributes storage_attributes_{}; + ColumnTypeAttributes type_attributes_{}; + std::string comment_{}; +}; + +// A builder class to facilitate creation of ColumnSchema instances. +class ColumnSchemaBuilder final { + public: + ColumnSchemaBuilder() = default; + + // Operator for implicit conversion to ColumnSchema. This is useful to + // get rid of trailing call to Build() when creating an in-place instance + // of ColumnSchemaBilder as an argument for a method accepting ColumnSchema + // as a parameter. + operator ColumnSchema() const { + return Build(); + } + + ColumnSchema Build() const { +#if DCHECK_IS_ON() + Validate(); +#endif + return ColumnSchema( + name_, + type_, + is_nullable_, + is_immutable_, + is_auto_incrementing_, + read_default_, + write_default_, + storage_attributes_, + type_attributes_, + comment_); + } + + std::unique_ptr<ColumnSchema> New() const { +#if DCHECK_IS_ON() + Validate(); +#endif + std::unique_ptr<ColumnSchema> ret(new ColumnSchema( + name_, + type_, + is_nullable_, + is_immutable_, + is_auto_incrementing_, + read_default_, + write_default_, + storage_attributes_, + type_attributes_, + comment_)); + return ret; + } + + ColumnSchemaBuilder& name(std::string name) { + name_ = std::move(name); + return *this; + } + + ColumnSchemaBuilder& type(DataType type) { + type_ = type; + return *this; + } + + ColumnSchemaBuilder& nullable(bool nullable) { + is_nullable_ = nullable; + return *this; + } + + ColumnSchemaBuilder& immutable(bool immutable) { + is_immutable_ = immutable; + return *this; + } + + ColumnSchemaBuilder& auto_incrementing(bool auto_incrementing) { + if (auto_incrementing) { + // If the data type is set explicitly for an auto-incrementing column, + // check if it's consistent. + DCHECK(type_ == DataType::UNKNOWN_DATA || type_ == DataType::INT64); + // Automatically set the proper type for an auto-incrementing column. + type_ = DataType::INT64; + } + is_auto_incrementing_ = auto_incrementing; + return *this; + } + + // Set default value used on read if the column was not present before alter. + // The value 'read_default' is copied and the copy is passed to the produced + // ColumnSchema upon calling Build() or New(). + ColumnSchemaBuilder& read_default(const void* read_default) { + DCHECK(type_ != DataType::UNKNOWN_DATA); + if (!read_default) { + read_default_.reset(); + } else { + if (write_default_ && write_default_.get() == read_default) { + read_default_ = write_default_; + } else { + read_default_ = std::make_shared<Variant>(type_, read_default); + } + } + return *this; + } + + // Set default value for the column cell if the value was not specified on insert. + // The value 'write_default' is copied and the copy is passed to the produced + // ColumnSchema upon calling Build() or New(). + ColumnSchemaBuilder& write_default(const void* write_default) { + DCHECK(type_ != DataType::UNKNOWN_DATA); + if (!write_default) { + write_default_.reset(); + } else { + if (read_default_ && read_default_.get() == write_default) { + write_default_ = read_default_; + } else { + write_default_ = std::make_shared<Variant>(type_, write_default); + } + } + return *this; + } + + ColumnSchemaBuilder& storage_attributes(ColumnStorageAttributes attributes) { + storage_attributes_ = attributes; + return *this; + } + + ColumnSchemaBuilder& type_attributes(ColumnTypeAttributes attributes) { + type_attributes_ = attributes; + return *this; + } + + ColumnSchemaBuilder& comment(std::string comment) { + comment_ = std::move(comment); + return *this; + } + + private: + +#if DCHECK_IS_ON() + void Validate() const { + // At least name and type should be set for a column. + DCHECK(!name_.empty()); + DCHECK(type_ != DataType::UNKNOWN_DATA); + + // Make sure the specified Variant data is consistent with the data type. + DCHECK(!read_default_ || type_ == read_default_->type()); + DCHECK(!write_default_ || type_ == write_default_->type()); + + // Extra sanity check for auto-incrementing columns. + if (is_auto_incrementing_) { + DCHECK_EQ(INT64, type_); + DCHECK(!is_nullable_); + DCHECK(!is_immutable_); + DCHECK(!read_default_); + DCHECK(!write_default_); + } + } +#endif // #if DCHECK_IS_ON() ... + + std::string name_{}; + DataType type_ = DataType::UNKNOWN_DATA; + bool is_nullable_ = false; + bool is_immutable_ = false; + bool is_auto_incrementing_ = false; + std::shared_ptr<Variant> read_default_{}; + std::shared_ptr<Variant> write_default_{}; + ColumnStorageAttributes storage_attributes_{}; + ColumnTypeAttributes type_attributes_{}; + std::string comment_{}; }; // The schema for a set of rows. @@ -1095,38 +1295,24 @@ class SchemaBuilder { Schema Build() const { return Schema(cols_, col_ids_, num_key_columns_); } Schema BuildWithoutIds() const { return Schema(cols_, num_key_columns_); } - Status AddKeyColumn(const std::string& name, DataType type); - - Status AddColumn(const ColumnSchema& column, bool is_key); + Status AddKeyColumn(const std::string& name, DataType type) { + return AddKeyColumn(ColumnSchemaBuilder().name(name).type(type)); + } + Status AddKeyColumn(const ColumnSchema& column) { + return AddColumn(column, /*is_key=*/true); + } Status AddColumn(const std::string& name, DataType type) { - return AddColumn(name, type, false, false, nullptr, nullptr); + return AddColumn(ColumnSchemaBuilder().name(name).type(type)); + } + Status AddColumn(const ColumnSchema& column) { + return AddColumn(column, /*is_key=*/false); } - Status AddNullableColumn(const std::string& name, DataType type) { - return AddColumn(name, type, true, false, nullptr, nullptr); - } - - Status AddColumn(const std::string& name, - DataType type, - bool is_nullable, - const void* read_default, - const void* write_default); - - Status AddColumn(const std::string& name, - DataType type, - bool is_nullable, - bool is_immutable, - const void* read_default, - const void* write_default); - - Status AddColumn(const std::string& name, - DataType type, - bool is_nullable, - bool is_immutable, - bool is_auto_incrementing, - const void* read_default, - const void* write_default); + return AddColumn(ColumnSchemaBuilder().name(name).type(type).nullable(true)); + } + + Status AddColumn(const ColumnSchema& column, bool is_key); Status RemoveColumn(const std::string& name); @@ -1135,13 +1321,13 @@ class SchemaBuilder { Status ApplyColumnSchemaDelta(const ColumnSchemaDelta& col_delta); private: - DISALLOW_COPY_AND_ASSIGN(SchemaBuilder); - ColumnId next_id_; std::vector<ColumnId> col_ids_; std::vector<ColumnSchema> cols_; std::unordered_set<std::string> col_names_; size_t num_key_columns_; + + DISALLOW_COPY_AND_ASSIGN(SchemaBuilder); }; } // namespace kudu diff --git a/src/kudu/common/wire_protocol-test-util.h b/src/kudu/common/wire_protocol-test-util.h index f3daf8638..9ab8ce9c1 100644 --- a/src/kudu/common/wire_protocol-test-util.h +++ b/src/kudu/common/wire_protocol-test-util.h @@ -15,8 +15,7 @@ // specific language governing permissions and limitations // under the License. -#ifndef KUDU_COMMON_WIRE_PROTOCOL_TEST_UTIL_H_ -#define KUDU_COMMON_WIRE_PROTOCOL_TEST_UTIL_H_ +#pragma once #include "kudu/common/wire_protocol.h" @@ -33,7 +32,7 @@ namespace kudu { inline Schema GetSimpleTestSchema() { return Schema({ ColumnSchema("key", INT32), ColumnSchema("int_val", INT32), - ColumnSchema("string_val", STRING, true) }, + ColumnSchema("string_val", STRING, ColumnSchema::NULLABLE) }, 1); } @@ -122,5 +121,3 @@ inline void AddTestKeyToPB(RowOperationsPB::Type op_type, } } // namespace kudu - -#endif /* KUDU_COMMON_WIRE_PROTOCOL_TEST_UTIL_H_ */ diff --git a/src/kudu/common/wire_protocol-test.cc b/src/kudu/common/wire_protocol-test.cc index 48e4539dd..44546803a 100644 --- a/src/kudu/common/wire_protocol-test.cc +++ b/src/kudu/common/wire_protocol-test.cc @@ -71,9 +71,9 @@ class WireProtocolTest : public KuduTest { public: WireProtocolTest() : schema_({ ColumnSchema("string", STRING), - ColumnSchema("nullable_string", STRING, /* is_nullable=*/true), + ColumnSchema("nullable_string", STRING, ColumnSchema::NULLABLE), ColumnSchema("int", INT32), - ColumnSchema("nullable_int", INT32, /* is_nullable=*/true), + ColumnSchema("nullable_int", INT32, ColumnSchema::NULLABLE), ColumnSchema("int64", INT64) }, 1), test_data_arena_(4096) { @@ -374,8 +374,9 @@ TEST_F(WireProtocolTest, TestColumnarRowBlockToPBWithPadding) { Schema tablet_schema({ ColumnSchema("key", UNIXTIME_MICROS), ColumnSchema("col1", STRING), ColumnSchema("col2", UNIXTIME_MICROS), - ColumnSchema("col3", INT32, true /* nullable */), - ColumnSchema("col4", UNIXTIME_MICROS, true /* nullable */)}, 1); + ColumnSchema("col3", INT32, ColumnSchema::NULLABLE), + ColumnSchema("col4", UNIXTIME_MICROS, ColumnSchema::NULLABLE) }, + 1); RowBlock block(&tablet_schema, kNumRows, &mem); block.selection_vector()->SetAllTrue(); @@ -399,8 +400,9 @@ TEST_F(WireProtocolTest, TestColumnarRowBlockToPBWithPadding) { Schema proj_schema({ ColumnSchema("col1", STRING), ColumnSchema("key", UNIXTIME_MICROS), ColumnSchema("col2", UNIXTIME_MICROS), - ColumnSchema("col4", UNIXTIME_MICROS, true /* nullable */), - ColumnSchema("col3", INT32, true /* nullable */)}, 0); + ColumnSchema("col4", UNIXTIME_MICROS, ColumnSchema::NULLABLE), + ColumnSchema("col3", INT32, ColumnSchema::NULLABLE)}, + 0); // Convert to PB. faststring direct, indirect; @@ -497,7 +499,8 @@ class WireProtocolBenchmark : for (const auto& c : spec.columns) { column_schemas.emplace_back(Substitute("col$0", i++), c.type, - /*nullable=*/c.null_fraction >= 0); + c.null_fraction >= 0 ? ColumnSchema::NULLABLE + : ColumnSchema::NOT_NULL); } CHECK_OK(benchmark_schema_.Reset(std::move(column_schemas), 0)); } @@ -716,47 +719,66 @@ TEST_F(WireProtocolTest, TestColumnDefaultValue) { ColumnSchema col1("col1", STRING); ColumnSchemaToPB(col1, &pb); - optional<ColumnSchema> col1fpb; - ASSERT_OK(ColumnSchemaFromPB(pb, &col1fpb)); - ASSERT_FALSE(col1fpb->has_read_default()); - ASSERT_FALSE(col1fpb->has_write_default()); - ASSERT_TRUE(col1fpb->read_default_value() == nullptr); - - ColumnSchema col2("col2", STRING, false, false, false, &read_default_str); + ColumnSchemaBuilder col1bld; + ASSERT_OK(ColumnSchemaBuilderFromPB(pb, &col1bld)); + ColumnSchema col1fpb(col1bld); + ASSERT_FALSE(col1fpb.has_read_default()); + ASSERT_FALSE(col1fpb.has_write_default()); + ASSERT_TRUE(col1fpb.read_default_value() == nullptr); + + ColumnSchema col2(ColumnSchemaBuilder() + .name("col2") + .type(STRING) + .read_default(&read_default_str)); ColumnSchemaToPB(col2, &pb); - optional<ColumnSchema> col2fpb; - ASSERT_OK(ColumnSchemaFromPB(pb, &col2fpb)); - ASSERT_TRUE(col2fpb->has_read_default()); - ASSERT_FALSE(col2fpb->has_write_default()); - ASSERT_EQ(read_default_str, *static_cast<const Slice *>(col2fpb->read_default_value())); - ASSERT_EQ(nullptr, static_cast<const Slice *>(col2fpb->write_default_value())); - - ColumnSchema col3("col3", STRING, false, false, false, &read_default_str, &write_default_str); + ColumnSchemaBuilder col2bld; + ASSERT_OK(ColumnSchemaBuilderFromPB(pb, &col2bld)); + ColumnSchema col2fpb(col2bld); + ASSERT_TRUE(col2fpb.has_read_default()); + ASSERT_FALSE(col2fpb.has_write_default()); + ASSERT_EQ(read_default_str, *static_cast<const Slice*>(col2fpb.read_default_value())); + ASSERT_EQ(nullptr, static_cast<const Slice*>(col2fpb.write_default_value())); + + ColumnSchema col3(ColumnSchemaBuilder() + .name("col3") + .type(STRING) + .read_default(&read_default_str) + .write_default(&write_default_str)); ColumnSchemaToPB(col3, &pb); - optional<ColumnSchema> col3fpb; - ASSERT_OK(ColumnSchemaFromPB(pb, &col3fpb)); - ASSERT_TRUE(col3fpb->has_read_default()); - ASSERT_TRUE(col3fpb->has_write_default()); - ASSERT_EQ(read_default_str, *static_cast<const Slice *>(col3fpb->read_default_value())); - ASSERT_EQ(write_default_str, *static_cast<const Slice *>(col3fpb->write_default_value())); - - ColumnSchema col4("col4", UINT32, false, false, false, &read_default_u32); + ColumnSchemaBuilder col3bld; + ASSERT_OK(ColumnSchemaBuilderFromPB(pb, &col3bld)); + ColumnSchema col3fpb(col3bld); + ASSERT_TRUE(col3fpb.has_read_default()); + ASSERT_TRUE(col3fpb.has_write_default()); + ASSERT_EQ(read_default_str, *static_cast<const Slice*>(col3fpb.read_default_value())); + ASSERT_EQ(write_default_str, *static_cast<const Slice*>(col3fpb.write_default_value())); + + ColumnSchema col4(ColumnSchemaBuilder() + .name("col4") + .type(UINT32) + .read_default(&read_default_u32)); ColumnSchemaToPB(col4, &pb); - optional<ColumnSchema> col4fpb; - ASSERT_OK(ColumnSchemaFromPB(pb, &col4fpb)); - ASSERT_TRUE(col4fpb->has_read_default()); - ASSERT_FALSE(col4fpb->has_write_default()); - ASSERT_EQ(read_default_u32, *static_cast<const uint32_t *>(col4fpb->read_default_value())); - ASSERT_EQ(nullptr, static_cast<const uint32_t *>(col4fpb->write_default_value())); - - ColumnSchema col5("col5", UINT32, false, false, false, &read_default_u32, &write_default_u32); + ColumnSchemaBuilder col4bld; + ASSERT_OK(ColumnSchemaBuilderFromPB(pb, &col4bld)); + ColumnSchema col4fpb(col4bld); + ASSERT_TRUE(col4fpb.has_read_default()); + ASSERT_FALSE(col4fpb.has_write_default()); + ASSERT_EQ(read_default_u32, *static_cast<const uint32_t*>(col4fpb.read_default_value())); + ASSERT_EQ(nullptr, static_cast<const uint32_t*>(col4fpb.write_default_value())); + + ColumnSchema col5(ColumnSchemaBuilder() + .name("col5") + .type(UINT32) + .read_default(&read_default_u32) + .write_default(&write_default_u32)); ColumnSchemaToPB(col5, &pb); - optional<ColumnSchema> col5fpb; - ASSERT_OK(ColumnSchemaFromPB(pb, &col5fpb)); - ASSERT_TRUE(col5fpb->has_read_default()); - ASSERT_TRUE(col5fpb->has_write_default()); - ASSERT_EQ(read_default_u32, *static_cast<const uint32_t *>(col5fpb->read_default_value())); - ASSERT_EQ(write_default_u32, *static_cast<const uint32_t *>(col5fpb->write_default_value())); + ColumnSchemaBuilder col5bld; + ASSERT_OK(ColumnSchemaBuilderFromPB(pb, &col5bld)); + ColumnSchema col5fpb(col5bld); + ASSERT_TRUE(col5fpb.has_read_default()); + ASSERT_TRUE(col5fpb.has_write_default()); + ASSERT_EQ(read_default_u32, *static_cast<const uint32_t*>(col5fpb.read_default_value())); + ASSERT_EQ(write_default_u32, *static_cast<const uint32_t*>(col5fpb.write_default_value())); } // Regression test for KUDU-2378; the call to ColumnSchemaFromPB yielded a crash. @@ -765,8 +787,8 @@ TEST_F(WireProtocolTest, TestCrashOnAlignedLoadOf128BitReadDefault) { pb.set_name("col"); pb.set_type(DECIMAL128); pb.set_read_default_value(string(16, 'a')); - optional<ColumnSchema> col; - ASSERT_OK(ColumnSchemaFromPB(pb, &col)); + ColumnSchemaBuilder bld; + ASSERT_OK(ColumnSchemaBuilderFromPB(pb, &bld)); } // Regression test for KUDU-2622; Validate read and write default value sizes. @@ -776,9 +798,9 @@ TEST_F(WireProtocolTest, TestInvalidReadAndWriteDefault) { pb.set_name("col"); pb.set_type(DECIMAL128); pb.set_read_default_value(string(8, 'a')); - optional<ColumnSchema> col; - Status s = ColumnSchemaFromPB(pb, &col); - EXPECT_TRUE(s.IsCorruption()); + ColumnSchemaBuilder bld; + const auto s = ColumnSchemaBuilderFromPB(pb, &bld); + EXPECT_TRUE(s.IsCorruption()) << s.ToString(); ASSERT_STR_CONTAINS(s.ToString(), "Corruption: Not enough bytes for decimal: read default"); } { @@ -786,9 +808,9 @@ TEST_F(WireProtocolTest, TestInvalidReadAndWriteDefault) { pb.set_name("col"); pb.set_type(DECIMAL128); pb.set_write_default_value(string(8, 'a')); - optional<ColumnSchema> col; - Status s = ColumnSchemaFromPB(pb, &col); - EXPECT_TRUE(s.IsCorruption()); + ColumnSchemaBuilder bld; + const auto s = ColumnSchemaBuilderFromPB(pb, &bld); + EXPECT_TRUE(s.IsCorruption()) << s.ToString(); ASSERT_STR_CONTAINS(s.ToString(), "Corruption: Not enough bytes for decimal: write default"); } } diff --git a/src/kudu/common/wire_protocol.cc b/src/kudu/common/wire_protocol.cc index f16f7f27b..1a24da9af 100644 --- a/src/kudu/common/wire_protocol.cc +++ b/src/kudu/common/wire_protocol.cc @@ -31,7 +31,6 @@ #include <glog/logging.h> #include <google/protobuf/map.h> -#include <google/protobuf/stubs/common.h> #include "kudu/common/column_predicate.h" #include "kudu/common/columnblock.h" @@ -275,36 +274,40 @@ void ColumnSchemaToPB(const ColumnSchema& col_schema, ColumnSchemaPB *pb, int fl } } -Status ColumnSchemaFromPB(const ColumnSchemaPB& pb, optional<ColumnSchema>* col_schema) { - const void *write_default_ptr = nullptr; - const void *read_default_ptr = nullptr; - Slice write_default; - Slice read_default; - const TypeInfo* typeinfo = GetTypeInfo(pb.type()); +Status ColumnSchemaBuilderFromPB(const ColumnSchemaPB& pb, + ColumnSchemaBuilder* csb) { + ColumnSchemaBuilder builder; + builder.name(pb.name()) + .type(pb.type()) + .nullable(pb.is_nullable()); + if (pb.has_read_default_value()) { - read_default = Slice(pb.read_default_value()); + Slice read_default(pb.read_default_value()); + const TypeInfo* typeinfo = GetTypeInfo(pb.type()); if (typeinfo->physical_type() == BINARY) { - read_default_ptr = &read_default; + builder.read_default(&read_default); } else { if (typeinfo->size() > read_default.size()) { return Status::Corruption( Substitute("Not enough bytes for $0: read default size ($1) less than type size ($2)", typeinfo->name(), read_default.size(), typeinfo->size())); } - read_default_ptr = read_default.data(); + builder.read_default(read_default.data()); } } + if (pb.has_write_default_value()) { - write_default = Slice(pb.write_default_value()); + Slice write_default(pb.write_default_value()); + const TypeInfo* typeinfo = GetTypeInfo(pb.type()); if (typeinfo->physical_type() == BINARY) { - write_default_ptr = &write_default; + builder.write_default(&write_default); } else { if (typeinfo->size() > write_default.size()) { return Status::Corruption( Substitute("Not enough bytes for $0: write default size ($1) less than type size ($2)", typeinfo->name(), write_default.size(), typeinfo->size())); } - write_default_ptr = write_default.data(); + builder.write_default(write_default.data()); } } @@ -321,6 +324,7 @@ Status ColumnSchemaFromPB(const ColumnSchemaPB& pb, optional<ColumnSchema>* col_ type_attributes.length = typeAttributesPB.length(); } } + builder.type_attributes(type_attributes); ColumnStorageAttributes attributes; if (pb.has_encoding()) { @@ -332,17 +336,15 @@ Status ColumnSchemaFromPB(const ColumnSchemaPB& pb, optional<ColumnSchema>* col_ if (pb.has_cfile_block_size()) { attributes.cfile_block_size = pb.cfile_block_size(); } + builder.storage_attributes(attributes); - // According to the URL below, the default value for strings that are optional - // in protobuf is the empty string. So, it's safe to use pb.comment() directly - // regardless of whether has_comment() is true or false. - // https://developers.google.com/protocol-buffers/docs/proto#optional - bool immutable = pb.has_immutable() ? pb.immutable() : false; - bool auto_incrementing = pb.has_is_auto_incrementing() ? pb.is_auto_incrementing() : false; - *col_schema = ColumnSchema(pb.name(), pb.type(), pb.is_nullable(), - immutable, auto_incrementing, - read_default_ptr, write_default_ptr, - attributes, type_attributes, pb.comment()); + builder.immutable(pb.has_immutable() ? pb.immutable() : false); + builder.auto_incrementing( + pb.has_is_auto_incrementing() ? pb.is_auto_incrementing() : false); + if (pb.has_comment()) { + builder.comment(pb.comment()); + } + *csb = std::move(builder); return Status::OK(); } @@ -414,9 +416,6 @@ Status ColumnPBsToSchema(const RepeatedPtrField<ColumnSchemaPB>& column_pbs, int num_key_columns = 0; bool is_handling_key = true; for (const ColumnSchemaPB& pb : column_pbs) { - optional<ColumnSchema> column; - RETURN_NOT_OK(ColumnSchemaFromPB(pb, &column)); - columns.emplace_back(std::move(*column)); if (pb.is_key()) { if (!is_handling_key) { return Status::InvalidArgument( @@ -426,6 +425,9 @@ Status ColumnPBsToSchema(const RepeatedPtrField<ColumnSchemaPB>& column_pbs, } else { is_handling_key = false; } + ColumnSchemaBuilder builder; + RETURN_NOT_OK(ColumnSchemaBuilderFromPB(pb, &builder)); + columns.emplace_back(builder.Build()); if (pb.has_id()) { column_ids.emplace_back(pb.id()); } diff --git a/src/kudu/common/wire_protocol.h b/src/kudu/common/wire_protocol.h index 737727bc2..bdb711a1f 100644 --- a/src/kudu/common/wire_protocol.h +++ b/src/kudu/common/wire_protocol.h @@ -38,6 +38,7 @@ namespace kudu { class Arena; class ColumnPredicate; class ColumnSchema; +class ColumnSchemaBuilder; class faststring; class HostPort; class RowBlock; @@ -110,9 +111,12 @@ Status SchemaFromPB(const SchemaPB& pb, Schema *schema); // 'flags' is a bitfield of SchemaPBConversionFlags values. void ColumnSchemaToPB(const ColumnSchema& schema, ColumnSchemaPB *pb, int flags = 0); -// Return the ColumnSchema created from the specified protobuf. -// If the column schema is invalid, return a non-OK status. -Status ColumnSchemaFromPB(const ColumnSchemaPB& pb, std::optional<ColumnSchema>* col_schema); +// Produce ColumnSchemaBuilder based on the specified protobuf 'pb', outputting +// the result builder object into the 'csb' output parameter. Calling Build() +// on the builder object yields corresponding ColumnSchema. +// If the column schema protobuf is invalid, return a non-OK status. +Status ColumnSchemaBuilderFromPB(const ColumnSchemaPB& pb, + ColumnSchemaBuilder* csb); // Convert the given list of ColumnSchemaPB objects into a Schema object. // diff --git a/src/kudu/integration-tests/auto_incrementing-itest.cc b/src/kudu/integration-tests/auto_incrementing-itest.cc index c22cc865f..1063265d8 100644 --- a/src/kudu/integration-tests/auto_incrementing-itest.cc +++ b/src/kudu/integration-tests/auto_incrementing-itest.cc @@ -187,10 +187,11 @@ class AutoIncrementingItest : public KuduTest { scan->set_order_mode(ORDERED); Schema schema = Schema({ ColumnSchema("c0", INT32), - ColumnSchema(Schema::GetAutoIncrementingColumnName(), - INT64, false,false, true), + ColumnSchemaBuilder() + .name(Schema::GetAutoIncrementingColumnName()) + .auto_incrementing(true), ColumnSchema("c1", STRING), - },2); + }, 2); RETURN_NOT_OK(SchemaToColumnPBs(schema, scan->mutable_projected_columns())); RETURN_NOT_OK(cluster_->tserver_proxy(ts)->Scan(req, &resp, &rpc)); tserver::TabletServerTestBase::StringifyRowsFromResponse(schema, rpc, &resp, results); diff --git a/src/kudu/integration-tests/create-table-itest.cc b/src/kudu/integration-tests/create-table-itest.cc index dc9185a14..c74eaaea7 100644 --- a/src/kudu/integration-tests/create-table-itest.cc +++ b/src/kudu/integration-tests/create-table-itest.cc @@ -283,7 +283,7 @@ TEST_F(CreateTableITest, TestSpreadReplicasEvenlyWithDimension) { Schema schema = Schema({ ColumnSchema("key1", INT32), ColumnSchema("key2", INT32), ColumnSchema("int_val", INT32), - ColumnSchema("string_val", STRING, true) }, 2); + ColumnSchema("string_val", STRING, ColumnSchema::NULLABLE) }, 2); auto client_schema = KuduSchema::FromSchema(schema); auto create_table_func = [](KuduClient* client, @@ -416,7 +416,7 @@ TEST_F(CreateTableITest, TestSpreadReplicas) { Schema schema = Schema({ ColumnSchema("key1", INT32), ColumnSchema("key2", INT32), ColumnSchema("int_val", INT32), - ColumnSchema("string_val", STRING, true) }, 2); + ColumnSchema("string_val", STRING, ColumnSchema::NULLABLE) }, 2); auto client_schema = KuduSchema::FromSchema(schema); auto create_table_func = [](KuduClient* client, diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc index ba42c889c..7f9879ade 100644 --- a/src/kudu/master/catalog_manager.cc +++ b/src/kudu/master/catalog_manager.cc @@ -2972,14 +2972,15 @@ Status CatalogManager::ApplyAlterSchemaSteps( RETURN_NOT_OK(ProcessColumnPBDefaults(&new_col_pb)); // Can't accept a NOT NULL column without a default. - optional<ColumnSchema> new_col; - RETURN_NOT_OK(ColumnSchemaFromPB(new_col_pb, &new_col)); - if (!new_col->is_nullable() && !new_col->has_read_default()) { + ColumnSchemaBuilder csb; + RETURN_NOT_OK(ColumnSchemaBuilderFromPB(new_col_pb, &csb)); + const auto new_col = csb.Build(); + if (!new_col.is_nullable() && !new_col.has_read_default()) { return Status::InvalidArgument( - Substitute("column `$0`: NOT NULL columns must have a default", new_col->name())); + Substitute("column `$0`: NOT NULL columns must have a default", new_col.name())); } - RETURN_NOT_OK(builder.AddColumn(*new_col, false)); + RETURN_NOT_OK(builder.AddColumn(new_col, false)); break; } diff --git a/src/kudu/master/master-test.cc b/src/kudu/master/master-test.cc index a0f85554f..6780e305d 100644 --- a/src/kudu/master/master-test.cc +++ b/src/kudu/master/master-test.cc @@ -3517,7 +3517,7 @@ TEST_F(MasterTest, TestTableSchemaMetrics) { req.mutable_table()->set_table_name(kTableName); AlterTableRequestPB::Step* step = req.add_alter_schema_steps(); step->set_type(AlterTableRequestPB::ADD_COLUMN); - ColumnSchemaToPB(ColumnSchema("c1", INT32, true), + ColumnSchemaToPB(ColumnSchema("c1", INT32, ColumnSchema::NULLABLE), step->mutable_add_column()->mutable_schema()); AlterTableResponsePB resp; diff --git a/src/kudu/tablet/all_types-scan-correctness-test.cc b/src/kudu/tablet/all_types-scan-correctness-test.cc index bc4d61526..37143c6c1 100644 --- a/src/kudu/tablet/all_types-scan-correctness-test.cc +++ b/src/kudu/tablet/all_types-scan-correctness-test.cc @@ -79,11 +79,18 @@ static const int kStrlen = 10; struct RowOpsBase { RowOpsBase(DataType type, EncodingType encoding) : type_(type), encoding_(encoding) { - schema_ = Schema({ColumnSchema("key", INT32), - ColumnSchema("val_a", type, true, false, false, nullptr, nullptr, - ColumnStorageAttributes(encoding, DEFAULT_COMPRESSION)), - ColumnSchema("val_b", type, true, false, false, nullptr, nullptr, - ColumnStorageAttributes(encoding, DEFAULT_COMPRESSION))}, 1); + schema_ = Schema({ ColumnSchema("key", INT32), + ColumnSchemaBuilder() + .name("val_a") + .type(type) + .nullable(true) + .storage_attributes({encoding, DEFAULT_COMPRESSION}), + ColumnSchemaBuilder() + .name("val_b") + .type(type) + .nullable(true) + .storage_attributes({encoding, DEFAULT_COMPRESSION}) + }, 1); } DataType type_; @@ -366,15 +373,30 @@ public: } SchemaBuilder builder(*tablet()->metadata()->schema()); ignore_result(builder.RemoveColumn("val_c")); - ASSERT_OK(builder.AddColumn("val_c", rowops_.type_, true, default_ptr, nullptr)); + ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() + .name("val_c") + .type(rowops_.type_) + .nullable(true) + .read_default(default_ptr))); AlterSchema(builder.Build()); - altered_schema_ = Schema({ColumnSchema("key", INT32), - ColumnSchema("val_a", rowops_.type_, true, false, false, nullptr, nullptr, - ColumnStorageAttributes(rowops_.encoding_, DEFAULT_COMPRESSION)), - ColumnSchema("val_b", rowops_.type_, true, false, false, nullptr, nullptr, - ColumnStorageAttributes(rowops_.encoding_, DEFAULT_COMPRESSION)), - ColumnSchema("val_c", rowops_.type_, true, false, false, default_ptr, nullptr, - ColumnStorageAttributes(rowops_.encoding_, DEFAULT_COMPRESSION))}, 1); + altered_schema_ = Schema({ ColumnSchema("key", INT32), + ColumnSchemaBuilder() + .name("val_a") + .type(rowops_.type_) + .nullable(true) + .storage_attributes({rowops_.encoding_, DEFAULT_COMPRESSION}), + ColumnSchemaBuilder() + .name("val_b") + .type(rowops_.type_) + .nullable(true) + .storage_attributes({rowops_.encoding_, DEFAULT_COMPRESSION}), + ColumnSchemaBuilder() + .name("val_c") + .type(rowops_.type_) + .nullable(true) + .storage_attributes({rowops_.encoding_, DEFAULT_COMPRESSION}) + .read_default(default_ptr) }, + 1); } // Scan the results of a query. Set "count" to the number of results satisfying the predicates. diff --git a/src/kudu/tablet/cfile_set-test.cc b/src/kudu/tablet/cfile_set-test.cc index b0a6a1841..bf6760afa 100644 --- a/src/kudu/tablet/cfile_set-test.cc +++ b/src/kudu/tablet/cfile_set-test.cc @@ -86,12 +86,15 @@ ScanSpec GetInListScanSpec(const ColumnSchema& col_schema, const vector<int>& va class TestCFileSet : public KuduRowSetTest { public: - TestCFileSet() : - KuduRowSetTest(Schema({ ColumnSchema("c0", INT32), - ColumnSchema("c1", INT32, false, false, false, - nullptr, nullptr, GetRLEStorage()), - ColumnSchema("c2", INT32, true) }, 1)) - {} + TestCFileSet() + : KuduRowSetTest(Schema({ ColumnSchema("c0", INT32), + ColumnSchemaBuilder() + .name("c1") + .type(INT32) + .storage_attributes(GetRLEStorage()), + ColumnSchema("c2", INT32, ColumnSchema::NULLABLE) }, + 1)) { + } void SetUp() override { KuduRowSetTest::SetUp(); diff --git a/src/kudu/tablet/compaction-test.cc b/src/kudu/tablet/compaction-test.cc index b78d01e94..18495df0f 100644 --- a/src/kudu/tablet/compaction-test.cc +++ b/src/kudu/tablet/compaction-test.cc @@ -1068,12 +1068,20 @@ TEST_F(TestCompaction, TestMergeMultipleSchemas) { // Add an int column with default int32_t default_c2 = 10; - ASSERT_OK(builder.AddColumn("c2", INT32, false, &default_c2, &default_c2)); + ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() + .name("c2") + .type(INT32) + .read_default(&default_c2) + .write_default(&default_c2))); schemas.emplace_back(builder.Build()); // add a string column with default Slice default_c3("Hello World"); - ASSERT_OK(builder.AddColumn("c3", STRING, false, &default_c3, &default_c3)); + ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() + .name("c3") + .type(STRING) + .read_default(&default_c3) + .write_default(&default_c3))); schemas.emplace_back(builder.Build()); NO_FATALS(DoMerge(schemas.back(), schemas)); diff --git a/src/kudu/tablet/delta_compaction-test.cc b/src/kudu/tablet/delta_compaction-test.cc index f6ced0bbd..d4980e5c4 100644 --- a/src/kudu/tablet/delta_compaction-test.cc +++ b/src/kudu/tablet/delta_compaction-test.cc @@ -117,12 +117,20 @@ TEST_F(TestDeltaCompaction, TestMergeMultipleSchemas) { // Add an int column with default uint32_t default_c2 = 10; - ASSERT_OK(builder.AddColumn("c2", UINT32, false, &default_c2, &default_c2)); + ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() + .name("c2") + .type(UINT32) + .read_default(&default_c2) + .write_default(&default_c2))); schemas.push_back(builder.Build()); // add a string column with default Slice default_c3("Hello World"); - ASSERT_OK(builder.AddColumn("c3", STRING, false, &default_c3, &default_c3)); + ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() + .name("c3") + .type(STRING) + .read_default(&default_c3) + .write_default(&default_c3))); schemas.push_back(builder.Build()); vector<shared_ptr<DeltaStore> > inputs; diff --git a/src/kudu/tablet/diff_scan-test.cc b/src/kudu/tablet/diff_scan-test.cc index f6c848d26..b2d454df8 100644 --- a/src/kudu/tablet/diff_scan-test.cc +++ b/src/kudu/tablet/diff_scan-test.cc @@ -114,10 +114,10 @@ TEST_P(DiffScanTest, TestDiffScan) { // The merge iterator requires an IS_DELETED column when including deleted // rows in order to support deduplication of the rows. - ASSERT_OK(builder.AddColumn("deleted", IS_DELETED, - /*is_nullable=*/ false, - /*read_default=*/ &kIsDeletedDefault, - /*write_default=*/ nullptr)); + ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() + .name("deleted") + .type(IS_DELETED) + .read_default(&kIsDeletedDefault))); } Schema projection = builder.BuildWithoutIds(); opts.projection = &projection; @@ -189,10 +189,10 @@ TEST_F(OrderedDiffScanWithDeletesTest, TestKudu3108) { opts.include_deleted_rows = true; static const bool kIsDeletedDefault = false; SchemaBuilder builder(*tablet->metadata()->schema()); - ASSERT_OK(builder.AddColumn("deleted", IS_DELETED, - /*is_nullable=*/ false, - /*read_default=*/ &kIsDeletedDefault, - /*write_default=*/ nullptr)); + ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() + .name("deleted") + .type(IS_DELETED) + .read_default(&kIsDeletedDefault))); Schema projection = builder.BuildWithoutIds(); opts.projection = &projection; diff --git a/src/kudu/tablet/diskrowset-test.cc b/src/kudu/tablet/diskrowset-test.cc index 4a52e25c3..ba07f34c4 100644 --- a/src/kudu/tablet/diskrowset-test.cc +++ b/src/kudu/tablet/diskrowset-test.cc @@ -920,9 +920,10 @@ TEST_P(DiffScanRowSetTest, TestFuzz) { } if (add_vc_is_deleted) { bool read_default = false; - col_schemas.emplace_back("is_deleted", IS_DELETED, /*is_nullable=*/ false, - /*is_immutable=*/ false, /*is_auto_incrementing=*/ false, - &read_default); + col_schemas.emplace_back(ColumnSchemaBuilder() + .name("is_deleted") + .type(IS_DELETED) + .read_default(&read_default)); col_ids.emplace_back(schema_.max_col_id() + 1); } Schema projection(col_schemas, col_ids, 1); diff --git a/src/kudu/tablet/key_value_test_schema.h b/src/kudu/tablet/key_value_test_schema.h index 5878285ac..e19eca6c0 100644 --- a/src/kudu/tablet/key_value_test_schema.h +++ b/src/kudu/tablet/key_value_test_schema.h @@ -59,7 +59,7 @@ struct ExpectedKeyValueRow { inline Schema CreateKeyValueTestSchema() { return Schema({ColumnSchema("key", INT32), - ColumnSchema("val", INT32, true) }, 1); + ColumnSchema("val", INT32, ColumnSchema::NULLABLE) }, 1); } inline std::ostream& operator<<(std::ostream& o, const ExpectedKeyValueRow& t) { diff --git a/src/kudu/tablet/memrowset-test.cc b/src/kudu/tablet/memrowset-test.cc index dc4caaace..7a6db8a1c 100644 --- a/src/kudu/tablet/memrowset-test.cc +++ b/src/kudu/tablet/memrowset-test.cc @@ -25,6 +25,7 @@ #include <ostream> #include <string> #include <tuple> +#include <type_traits> #include <unordered_set> #include <vector> @@ -699,9 +700,10 @@ TEST_P(ParameterizedTestMemRowSet, TestScanSnapToExclude) { unique_ptr<SchemaBuilder> sb = CreateSchemaBuilder(); if (add_vc_is_deleted) { const bool kFalse = false; - ASSERT_OK(sb->AddColumn("deleted", IS_DELETED, - /*is_nullable=*/false, - &kFalse, /*write_default=*/nullptr)); + ASSERT_OK(sb->AddColumn(ColumnSchemaBuilder() + .name("deleted") + .type(IS_DELETED) + .read_default(&kFalse))); } Schema projection = sb->Build(); RowIteratorOptions opts; @@ -786,9 +788,10 @@ TEST_F(TestMemRowSet, TestScanVirtualColumnIsDeleted) { ASSERT_OK(sb.AddKeyColumn("key", STRING)); ASSERT_OK(sb.AddColumn("val", UINT32)); const bool kFalse = false; - ASSERT_OK(sb.AddColumn("deleted", IS_DELETED, - /*is_nullable=*/false, - &kFalse, /*write_default=*/nullptr)); + ASSERT_OK(sb.AddColumn(ColumnSchemaBuilder() + .name("deleted") + .type(IS_DELETED) + .read_default(&kFalse))); Schema projection = sb.Build(); RowIteratorOptions opts; diff --git a/src/kudu/tablet/tablet-decoder-eval-test.cc b/src/kudu/tablet/tablet-decoder-eval-test.cc index 87d85aa24..a9b0f39a1 100644 --- a/src/kudu/tablet/tablet-decoder-eval-test.cc +++ b/src/kudu/tablet/tablet-decoder-eval-test.cc @@ -75,15 +75,18 @@ class TabletDecoderEvalTest : public KuduTabletTest, public: TabletDecoderEvalTest() : KuduTabletTest(Schema({ColumnSchema("key", INT32), - ColumnSchema("string_val_a", STRING, true, false, - false, nullptr, nullptr, - ColumnStorageAttributes(DICT_ENCODING, - DEFAULT_COMPRESSION)), - ColumnSchema("string_val_b", STRING, true, false, - false, nullptr, nullptr, - ColumnStorageAttributes(DICT_ENCODING, - DEFAULT_COMPRESSION))}, 1)) - {} + ColumnSchemaBuilder() + .name("string_val_a") + .type(STRING) + .nullable(true) + .storage_attributes({DICT_ENCODING, DEFAULT_COMPRESSION}), + ColumnSchemaBuilder() + .name("string_val_b") + .type(STRING) + .nullable(true) + .storage_attributes({DICT_ENCODING, DEFAULT_COMPRESSION}) + }, 1)) { + } void SetUp() override { KuduTabletTest::SetUp(); diff --git a/src/kudu/tablet/tablet-pushdown-test.cc b/src/kudu/tablet/tablet-pushdown-test.cc index 130a832b7..71704db09 100644 --- a/src/kudu/tablet/tablet-pushdown-test.cc +++ b/src/kudu/tablet/tablet-pushdown-test.cc @@ -21,6 +21,7 @@ #include <memory> #include <ostream> #include <string> +#include <type_traits> #include <unordered_map> #include <utility> #include <vector> @@ -39,7 +40,6 @@ #include "kudu/gutil/stringprintf.h" #include "kudu/tablet/local_tablet_writer.h" #include "kudu/tablet/tablet-test-util.h" -#include "kudu/tablet/tablet.h" #include "kudu/util/memory/arena.h" #include "kudu/util/status.h" #include "kudu/util/stopwatch.h" @@ -228,7 +228,7 @@ class TabletSparsePushdownTest : public KuduTabletTest { public: TabletSparsePushdownTest() : KuduTabletTest(Schema({ ColumnSchema("key", INT32), - ColumnSchema("val", INT32, true) }, + ColumnSchema("val", INT32, ColumnSchema::NULLABLE) }, 1)) { } diff --git a/src/kudu/tablet/tablet-schema-test.cc b/src/kudu/tablet/tablet-schema-test.cc index 1886f81ab..8ac99ca87 100644 --- a/src/kudu/tablet/tablet-schema-test.cc +++ b/src/kudu/tablet/tablet-schema-test.cc @@ -19,6 +19,7 @@ #include <cstdint> #include <memory> #include <string> +#include <type_traits> #include <utility> #include <vector> @@ -147,7 +148,11 @@ TEST_F(TestTabletSchema, TestWrite) { const int32_t c2_read_default = 7; SchemaBuilder builder(*tablet()->metadata()->schema()); - ASSERT_OK(builder.AddColumn("c2", INT32, false, &c2_read_default, &c2_write_default)); + ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() + .name("c2") + .type(INT32) + .read_default(&c2_read_default) + .write_default(&c2_write_default))); AlterSchema(builder.Build()); Schema s2 = builder.BuildWithoutIds(); @@ -190,7 +195,11 @@ TEST_F(TestTabletSchema, TestReInsert) { const int32_t c2_read_default = 7; SchemaBuilder builder(*tablet()->metadata()->schema()); - ASSERT_OK(builder.AddColumn("c2", INT32, false, &c2_read_default, &c2_write_default)); + ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() + .name("c2") + .type(INT32) + .read_default(&c2_read_default) + .write_default(&c2_write_default))); AlterSchema(builder.Build()); Schema s2 = builder.BuildWithoutIds(); diff --git a/src/kudu/tablet/tablet-test-base.h b/src/kudu/tablet/tablet-test-base.h index 1f3d9c1c9..55b7cbaac 100644 --- a/src/kudu/tablet/tablet-test-base.h +++ b/src/kudu/tablet/tablet-test-base.h @@ -247,20 +247,20 @@ struct NullableValueTestSetup { static Schema CreateSchema() { return Schema({ ColumnSchema("key", INT32), ColumnSchema("key_idx", INT32), - ColumnSchema("val", INT32, true) }, 1); + ColumnSchema("val", INT32, ColumnSchema::NULLABLE) }, 1); } - void BuildRowKey(KuduPartialRow *row, int64_t i) { + static void BuildRowKey(KuduPartialRow *row, int64_t i) { CHECK_OK(row->SetInt32(0, (int32_t)i)); } // builds a row key from an existing row for updates template<class RowType> - void BuildRowKeyFromExistingRow(KuduPartialRow *row, const RowType& src_row) { + static void BuildRowKeyFromExistingRow(KuduPartialRow *row, const RowType& src_row) { CHECK_OK(row->SetInt32(0, *reinterpret_cast<const int32_t*>(src_row.cell_ptr(0)))); } - void BuildRow(KuduPartialRow *row, int64_t key_idx, int32_t val = 0) { + static void BuildRow(KuduPartialRow *row, int64_t key_idx, int32_t val = 0) { BuildRowKey(row, key_idx); CHECK_OK(row->SetInt32(1, key_idx)); if (ShouldInsertAsNull(key_idx)) { @@ -270,7 +270,7 @@ struct NullableValueTestSetup { } } - std::string FormatDebugRow(int64_t key_idx, int64_t val, bool updated) { + static std::string FormatDebugRow(int64_t key_idx, int64_t val, bool updated) { if (!updated && ShouldInsertAsNull(key_idx)) { return strings::Substitute( "(int32 key=$0, int32 key_idx=$1, int32 val=NULL)", @@ -286,7 +286,7 @@ struct NullableValueTestSetup { return (key_idx & 2) != 0; } - uint64_t GetMaxRows() const { + static uint64_t GetMaxRows() { return std::numeric_limits<uint32_t>::max() - 1; } }; @@ -297,7 +297,12 @@ struct ImmutableColumnTestSetup { return Schema({ ColumnSchema("key", INT32), ColumnSchema("key_idx", INT32), ColumnSchema("val", INT32), - ColumnSchema("imm_val", INT32, true, true) }, 1); + ColumnSchemaBuilder() + .name("imm_val") + .type(INT32) + .nullable(true) + .immutable(true) }, + 1); } static void BuildRowKey(KuduPartialRow *row, int64_t i) { diff --git a/src/kudu/tablet/tablet-test-util.h b/src/kudu/tablet/tablet-test-util.h index ba2bbb970..e8e1697d9 100644 --- a/src/kudu/tablet/tablet-test-util.h +++ b/src/kudu/tablet/tablet-test-util.h @@ -791,9 +791,10 @@ static inline Schema GetRandomProjection(const Schema& schema, // Add a IS_DELETED virtual column some of the time. if (allow == AllowIsDeleted::YES && prng->Uniform(10) == 0) { bool read_default = false; - projected_cols.emplace_back("is_deleted", IS_DELETED, /*is_nullable=*/ false, - /*is_immutable=*/ false, /*is_auto_incrementing*/ false, - &read_default); + projected_cols.emplace_back(ColumnSchemaBuilder() + .name("is_deleted") + .type(IS_DELETED) + .read_default(&read_default)); projected_col_ids.emplace_back(schema.max_col_id() + 1); } return Schema(projected_cols, projected_col_ids, 0); diff --git a/src/kudu/tablet/tablet-test.cc b/src/kudu/tablet/tablet-test.cc index c7f2b0345..5520fffa2 100644 --- a/src/kudu/tablet/tablet-test.cc +++ b/src/kudu/tablet/tablet-test.cc @@ -1704,9 +1704,10 @@ TYPED_TEST(TestTablet, TestDiffScanUnobservableOperations) { // Create a projection with an IS_DELETED virtual column. vector<ColumnSchema> col_schemas(this->client_schema().columns()); bool read_default = false; - col_schemas.emplace_back("is_deleted", IS_DELETED, /*is_nullable=*/ false, - /*is_immutable=*/ false, /*is_auto_incremented=*/ false, - &read_default); + col_schemas.emplace_back(ColumnSchemaBuilder() + .name("is_deleted") + .type(IS_DELETED) + .read_default(&read_default)); Schema projection(col_schemas, this->client_schema().num_key_columns()); // Do the diff scan. diff --git a/src/kudu/tablet/tablet_auto_incrementing-test.cc b/src/kudu/tablet/tablet_auto_incrementing-test.cc index e3ea6090c..7957cb13e 100644 --- a/src/kudu/tablet/tablet_auto_incrementing-test.cc +++ b/src/kudu/tablet/tablet_auto_incrementing-test.cc @@ -43,9 +43,11 @@ namespace tablet { namespace { Schema CreateAutoIncrementingTestSchema() { - return Schema({ColumnSchema("key", INT64, false, false, - /*is_auto_incrementing*/ true, nullptr, nullptr, {}, {}, ""), - ColumnSchema("val", INT32, true) }, 1); + return Schema({ColumnSchemaBuilder() + .name("key") + .auto_incrementing(true), + ColumnSchema("val", INT32, ColumnSchema::NULLABLE) + }, 1); } } // anonymous namespace diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc index 211dd3991..28db0b46c 100644 --- a/src/kudu/tools/kudu-tool-test.cc +++ b/src/kudu/tools/kudu-tool-test.cc @@ -2849,8 +2849,8 @@ TEST_F(ToolTest, TestWalDumpWithAlterSchema) { tserver::AlterSchemaRequestPB* alter_schema = replicate->get()->mutable_alter_schema_request(); SchemaBuilder schema_builder = SchemaBuilder(schema); - ASSERT_OK(schema_builder.AddColumn(kAddColumnName1, STRING, true, nullptr, nullptr)); - ASSERT_OK(schema_builder.AddColumn(kAddColumnName2, STRING, true, nullptr, nullptr)); + ASSERT_OK(schema_builder.AddNullableColumn(kAddColumnName1, STRING)); + ASSERT_OK(schema_builder.AddNullableColumn(kAddColumnName2, STRING)); schema = schema_builder.BuildWithoutIds(); schema_with_ids = SchemaBuilder(schema).Build(); ASSERT_OK(SchemaToPB(schema_with_ids, alter_schema->mutable_schema())); @@ -3690,15 +3690,9 @@ void ToolTest::RunLoadgen(int num_tservers, ColumnSchema("int64_val", INT64), ColumnSchema("float_val", FLOAT), ColumnSchema("double_val", DOUBLE), - ColumnSchema("decimal32_val", DECIMAL32, false, false, false, - nullptr, nullptr, ColumnStorageAttributes(), - ColumnTypeAttributes(9, 9)), - ColumnSchema("decimal64_val", DECIMAL64, false, false, false, - nullptr, nullptr, ColumnStorageAttributes(), - ColumnTypeAttributes(18, 2)), - ColumnSchema("decimal128_val", DECIMAL128, false, false, false, - nullptr, nullptr, ColumnStorageAttributes(), - ColumnTypeAttributes(38, 0)), + ColumnSchemaBuilder().name("decimal32_val").type(DECIMAL32).type_attributes({9, 9}), + ColumnSchemaBuilder().name("decimal64_val").type(DECIMAL64).type_attributes({18, 2}), + ColumnSchemaBuilder().name("decimal128_val").type(DECIMAL128).type_attributes({38, 0}), ColumnSchema("unixtime_micros_val", UNIXTIME_MICROS), ColumnSchema("string_val", STRING), ColumnSchema("binary_val", BINARY), diff --git a/src/kudu/tserver/tablet_server-test.cc b/src/kudu/tserver/tablet_server-test.cc index e47b0ec9b..cddccf195 100644 --- a/src/kudu/tserver/tablet_server-test.cc +++ b/src/kudu/tserver/tablet_server-test.cc @@ -3531,10 +3531,10 @@ TEST_F(ScannerScansTest, TestDiffScan) { // Build a projection with an IS_DELETED column. SchemaBuilder builder(*tablet_replica_->tablet()->schema()); const bool kIsDeletedDefault = false; - ASSERT_OK(builder.AddColumn("is_deleted", IS_DELETED, - /*is_nullable=*/ false, - /*read_default=*/ &kIsDeletedDefault, - /*write_default=*/ nullptr)); + ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() + .name("is_deleted") + .type(IS_DELETED) + .read_default(&kIsDeletedDefault))); Schema projection = builder.BuildWithoutIds(); // Start scan. @@ -3599,10 +3599,10 @@ TEST_F(ScannerScansTest, TestDiffScanErrors) { // Build a projection with an IS_DELETED column. SchemaBuilder builder(*tablet_replica_->tablet()->schema()); const bool kIsDeletedDefault = false; - ASSERT_OK(builder.AddColumn("is_deleted", IS_DELETED, - /*is_nullable=*/ false, - /*read_default=*/ &kIsDeletedDefault, - /*write_default=*/ nullptr)); + ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() + .name("is_deleted") + .type(IS_DELETED) + .read_default(&kIsDeletedDefault))); Schema projection = builder.BuildWithoutIds(); ScanRequestPB req; @@ -3721,8 +3721,9 @@ TEST_F(ScannerScansTest, TestInvalidScanRequest_BadProjectionTypes) { // Verify mismatched nullability for the not-null int field ASSERT_OK( - projection.Reset({ ColumnSchema("int_val", INT32, true) }, // should be NOT NULL + projection.Reset({ ColumnSchema("int_val", INT32, ColumnSchema::NULLABLE) }, 0)); + // should be NOT NULL VerifyScanRequestFailure(projection, TabletServerErrorPB::MISMATCHED_SCHEMA, "The column 'int_val' must have type INT32 NOT " @@ -3730,8 +3731,9 @@ TEST_F(ScannerScansTest, TestInvalidScanRequest_BadProjectionTypes) { // Verify mismatched nullability for the nullable string field ASSERT_OK( - projection.Reset({ ColumnSchema("string_val", STRING, false) }, // should be NULLABLE + projection.Reset({ ColumnSchema("string_val", STRING, ColumnSchema::NOT_NULL) }, 0)); + // should be NULLABLE VerifyScanRequestFailure(projection, TabletServerErrorPB::MISMATCHED_SCHEMA, "The column 'string_val' must have type STRING " @@ -3739,8 +3741,9 @@ TEST_F(ScannerScansTest, TestInvalidScanRequest_BadProjectionTypes) { // Verify mismatched type for the not-null int field ASSERT_OK( - projection.Reset({ ColumnSchema("int_val", INT16, false) }, // should be INT32 NOT NULL + projection.Reset({ ColumnSchema("int_val", INT16, ColumnSchema::NOT_NULL) }, 0)); + // should be INT32 NOT NULL VerifyScanRequestFailure(projection, TabletServerErrorPB::MISMATCHED_SCHEMA, "The column 'int_val' must have type INT32 NOT " @@ -3748,8 +3751,9 @@ TEST_F(ScannerScansTest, TestInvalidScanRequest_BadProjectionTypes) { // Verify mismatched type for the nullable string field ASSERT_OK(projection.Reset( - { ColumnSchema("string_val", INT32, true) }, // should be STRING NULLABLE + { ColumnSchema("string_val", INT32, ColumnSchema::NULLABLE) }, 0)); + // should be STRING NULLABLE VerifyScanRequestFailure(projection, TabletServerErrorPB::MISMATCHED_SCHEMA, "The column 'string_val' must have type STRING " @@ -4051,7 +4055,11 @@ TEST_F(TabletServerTest, TestAlterSchema) { const int32_t c2_write_default = 5; const int32_t c2_read_default = 7; SchemaBuilder builder(schema_); - ASSERT_OK(builder.AddColumn("c2", INT32, false, &c2_read_default, &c2_write_default)); + ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() + .name("c2") + .type(INT32) + .read_default(&c2_read_default) + .write_default(&c2_write_default))); Schema s2 = builder.Build(); req.set_dest_uuid(mini_server_->server()->fs_manager()->uuid()); @@ -4106,7 +4114,10 @@ TEST_F(TabletServerTest, TestAlterSchema_AddColWithoutWriteDefault) { // Add a column with a read-default but no write-default. const uint32_t c2_read_default = 7; SchemaBuilder builder(schema_); - ASSERT_OK(builder.AddColumn("c2", INT32, false, &c2_read_default, nullptr)); + ASSERT_OK(builder.AddColumn(ColumnSchemaBuilder() + .name("c2") + .type(INT32) + .read_default(&c2_read_default))); Schema s2 = builder.Build(); req.set_dest_uuid(mini_server_->server()->fs_manager()->uuid()); diff --git a/src/kudu/tserver/tablet_server_authorization-test.cc b/src/kudu/tserver/tablet_server_authorization-test.cc index 1255505b7..31915b8fd 100644 --- a/src/kudu/tserver/tablet_server_authorization-test.cc +++ b/src/kudu/tserver/tablet_server_authorization-test.cc @@ -474,10 +474,17 @@ string GenerateEncodedKey(int32_t val, const Schema& schema) { // Returns a column schema PB that matches 'col', but has a different name. void MisnamedColumnSchemaToPB(const ColumnSchema& col, ColumnSchemaPB* pb) { - ColumnSchemaToPB(ColumnSchema(kDummyColumn, col.type_info()->physical_type(), col.is_nullable(), - col.is_immutable(), col.is_auto_incrementing(), - col.read_default_value(), col.write_default_value(), - col.attributes(), col.type_attributes()), pb); + ColumnSchemaToPB(ColumnSchemaBuilder() + .name(kDummyColumn) + .type(col.type_info()->physical_type()) + .nullable(col.is_nullable()) + .immutable(col.is_immutable()) + .auto_incrementing(col.is_auto_incrementing()) + .read_default(col.read_default_value()) + .write_default(col.write_default_value()) + .storage_attributes(col.attributes()) + .type_attributes(col.type_attributes()), + pb); } } // anonymous namespace @@ -509,8 +516,7 @@ class ScanPrivilegeAuthzTest : public AuthzTabletServerTestBase, } for (int i = 0; i < kNumVals; i++) { const string val = Substitute("val$0", kNumKeys + i); - ASSERT_OK(schema_builder.AddColumn(ColumnSchema(val, DataType::INT32), - /*is_key=*/false)); + ASSERT_OK(schema_builder.AddColumn(ColumnSchema(val, DataType::INT32))); col_names_.emplace_back(val); } schema_ = schema_builder.Build(); @@ -594,10 +600,11 @@ class ScanPrivilegeAuthzTest : public AuthzTabletServerTestBase, if (special_col == SpecialColumn::VIRTUAL) { auto* projected_column = pb.add_projected_columns(); bool default_bool = false; - ColumnSchemaToPB(ColumnSchema("is_deleted", DataType::IS_DELETED, /*is_nullable=*/false, - /*is_immutable=*/false, - /*is_auto_incrementing=*/false, - /*read_default=*/&default_bool, nullptr), projected_column); + ColumnSchemaToPB(ColumnSchemaBuilder() + .name("is_deleted") + .type(DataType::IS_DELETED) + .read_default(&default_bool), + projected_column); } CHECK_OK(GenerateScanAuthzToken(privilege, pb.mutable_authz_token())); return pb; diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc index 6430853f8..8de6e6a6b 100644 --- a/src/kudu/tserver/tablet_service.cc +++ b/src/kudu/tserver/tablet_service.cc @@ -480,25 +480,26 @@ static bool GetScanPrivilegesOrRespond(const NewScanRequestPB& scan_pb, const Sc unordered_set<ColumnId> required_privileges; // Determine the scan's projected key column IDs. for (int i = 0; i < scan_pb.projected_columns_size(); i++) { - optional<ColumnSchema> projected_column; - Status s = ColumnSchemaFromPB(scan_pb.projected_columns(i), &projected_column); - if (PREDICT_FALSE(!s.ok())) { + ColumnSchemaBuilder builder; + if (auto s = ColumnSchemaBuilderFromPB(scan_pb.projected_columns(i), &builder); + PREDICT_FALSE(!s.ok())) { LOG(WARNING) << s.ToString(); context->RespondRpcFailure(ErrorStatusPB::ERROR_INVALID_REQUEST, s); return false; } + const auto projected_column(builder.Build()); // A projection may contain virtual columns, which don't exist in the // tablet schema. If we were to search for a virtual column, we would // incorrectly get a "not found" error. To reconcile this with the fact // that we want to return an authorization error if the user has requested // a non-virtual column that doesn't exist, we require full scan privileges // for virtual columns. - if (projected_column->type_info()->is_virtual()) { + if (projected_column.type_info()->is_virtual()) { *required_column_privileges = unordered_set<ColumnId>(schema.column_ids().begin(), schema.column_ids().end()); return true; } - int col_idx = schema.find_column(projected_column->name()); + int col_idx = schema.find_column(projected_column.name()); if (col_idx == Schema::kColumnNotFound) { respond_not_authorized(scan_pb.projected_columns(i).name()); return false; @@ -2674,24 +2675,27 @@ static Status SetupScanSpec(const NewScanRequestPB& scan_pb, string("Invalid predicate ") + SecureShortDebugString(pred_pb) + ": has no lower or upper bound."); } - optional<ColumnSchema> col; - RETURN_NOT_OK(ColumnSchemaFromPB(pred_pb.column(), &col)); + ColumnSchemaBuilder builder; + RETURN_NOT_OK(ColumnSchemaBuilderFromPB(pred_pb.column(), &builder)); + const auto column_schema(builder.Build()); const void* lower_bound = nullptr; const void* upper_bound = nullptr; if (pred_pb.has_lower_bound()) { - RETURN_NOT_OK(ExtractPredicateValue(*col, pred_pb.lower_bound(), + RETURN_NOT_OK(ExtractPredicateValue(column_schema, + pred_pb.lower_bound(), scanner->arena(), &lower_bound)); } if (pred_pb.has_inclusive_upper_bound()) { - RETURN_NOT_OK(ExtractPredicateValue(*col, pred_pb.inclusive_upper_bound(), + RETURN_NOT_OK(ExtractPredicateValue(column_schema, + pred_pb.inclusive_upper_bound(), scanner->arena(), &upper_bound)); } - auto pred = ColumnPredicate::InclusiveRange(*col, lower_bound, upper_bound, scanner->arena()); - if (pred) { + if (const auto pred = ColumnPredicate::InclusiveRange( + column_schema, lower_bound, upper_bound, scanner->arena()); pred) { VLOG(3) << Substitute("Parsed predicate $0 from $1", pred->ToString(), SecureShortDebugString(scan_pb)); spec->AddPredicate(*pred);
