This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new 9e3901509 KUDU-1261 introduce ARRAY_1D_COLUMN_TYPE feature for masters
9e3901509 is described below
commit 9e39015095f5a8a3f33bc87fe19617c7d3caf410
Author: Alexey Serbin <[email protected]>
AuthorDate: Thu Oct 16 22:22:40 2025 -0700
KUDU-1261 introduce ARRAY_1D_COLUMN_TYPE feature for masters
This changelist introduces a new feature flag ARRAY_1D_COLUMN_TYPE
for Kudu masters to reflect whether one-dimensional array columns
are supported by the system catalog. Also, the C++ client code
is updated to require the feature flag at the server side when sending
requests to create a table with array type columns or adding an array
type column into already existing tables.
With this patch, attempts to create a new table with an array column
using a kudu-master without the necessary functionality (e.g., bits
compiled from the 1.18.x branch) result in proper behavior, and a
proper, actionable error is reported by the client:
Not implemented: Error creating table array_table on the master: cluster
does not support CreateTable with feature(s) ARRAY_1D_COLUMN_TYPE
Without this patch, an attempt to create the same table would result
in an error as well, but it's quite cryptic and not actionable, e.g.:
Remote error: Error creating table array_table on the master: Invalid
argument: invalid parameter for call kudu.master.MasterService.CreateTable:
missing fields: schema.columns[3].type, schema.columns[4].type
I'm planning to post a separate patch to update the Java client as well.
Change-Id: I61ab0d7446409ad924454d2d272bb02512bdffc8
Reviewed-on: http://gerrit.cloudera.org:8080/23556
Reviewed-by: Abhishek Chennaka <[email protected]>
Tested-by: Alexey Serbin <[email protected]>
---
src/kudu/client/CMakeLists.txt | 1 +
src/kudu/client/client-internal.cc | 17 +++-
src/kudu/client/client-internal.h | 6 +-
src/kudu/client/client.cc | 10 +-
src/kudu/client/service-features-test.cc | 150 ++++++++++++++++++++++++++++++
src/kudu/client/table_alterer-internal.cc | 6 +-
src/kudu/client/table_alterer-internal.h | 5 +-
src/kudu/common/schema.cc | 7 ++
src/kudu/common/schema.h | 13 +++
src/kudu/master/master.proto | 3 +
src/kudu/master/master_service.cc | 8 ++
11 files changed, 215 insertions(+), 11 deletions(-)
diff --git a/src/kudu/client/CMakeLists.txt b/src/kudu/client/CMakeLists.txt
index 7255d7c17..a5653289d 100644
--- a/src/kudu/client/CMakeLists.txt
+++ b/src/kudu/client/CMakeLists.txt
@@ -293,3 +293,4 @@ ADD_KUDU_TEST(client-unittest)
ADD_KUDU_TEST(flex_partitioning_client-test)
ADD_KUDU_TEST(predicate-test NUM_SHARDS 4)
ADD_KUDU_TEST(scan_token-test PROCESSORS 2)
+ADD_KUDU_TEST(service-features-test PROCESSORS 2)
diff --git a/src/kudu/client/client-internal.cc
b/src/kudu/client/client-internal.cc
index fbd6048b1..82deaef6b 100644
--- a/src/kudu/client/client-internal.cc
+++ b/src/kudu/client/client-internal.cc
@@ -36,7 +36,6 @@
#include <boost/container/vector.hpp>
#include <gflags/gflags_declare.h>
#include <glog/logging.h>
-#include <google/protobuf/stubs/common.h>
#include "kudu/client/authz_token_cache.h"
#include "kudu/client/master_proxy_rpc.h"
@@ -328,7 +327,8 @@ Status KuduClient::Data::CreateTable(KuduClient* client,
bool has_range_partition_bounds,
bool has_range_specific_hash_schema,
bool has_immutable_column_schema,
- bool has_auto_incrementing_column) {
+ bool has_auto_incrementing_column,
+ bool has_nested_columns) {
vector<uint32_t> features;
if (has_range_partition_bounds) {
features.push_back(MasterFeatures::RANGE_PARTITION_BOUNDS);
@@ -342,6 +342,13 @@ Status KuduClient::Data::CreateTable(KuduClient* client,
if (has_auto_incrementing_column) {
features.push_back(MasterFeatures::AUTO_INCREMENTING_COLUMN);
}
+ if (has_nested_columns) {
+ // As of time of writing, a nested column can only be of 1D array type.
+ // When more nested types are supported, the 'has_nested_columns' parameter
+ // needs to become an integer where different bits stands for particular
+ // features required at the server-side.
+ features.push_back(MasterFeatures::ARRAY_1D_COLUMN_TYPE);
+ }
Synchronizer sync;
AsyncLeaderMasterRpc<CreateTableRequestPB, CreateTableResponsePB> rpc(
deadline, client, BackoffType::EXPONENTIAL, req, resp,
@@ -434,7 +441,8 @@ Status KuduClient::Data::AlterTable(KuduClient* client,
const MonoTime& deadline,
bool has_add_drop_partition,
bool adding_range_with_custom_hash_schema,
- bool has_immutable_column_schema) {
+ bool has_immutable_column_schema,
+ bool has_nested_columns) {
vector<uint32_t> required_feature_flags;
if (has_add_drop_partition) {
required_feature_flags.push_back(MasterFeatures::ADD_DROP_RANGE_PARTITIONS);
@@ -445,6 +453,9 @@ Status KuduClient::Data::AlterTable(KuduClient* client,
if (has_immutable_column_schema) {
required_feature_flags.push_back(MasterFeatures::IMMUTABLE_COLUMN_ATTRIBUTE);
}
+ if (has_nested_columns) {
+ required_feature_flags.push_back(MasterFeatures::ARRAY_1D_COLUMN_TYPE);
+ }
Synchronizer sync;
AsyncLeaderMasterRpc<AlterTableRequestPB, AlterTableResponsePB> rpc(
deadline, client, BackoffType::EXPONENTIAL, req, resp,
diff --git a/src/kudu/client/client-internal.h
b/src/kudu/client/client-internal.h
index 2bf217794..ebac4bcc3 100644
--- a/src/kudu/client/client-internal.h
+++ b/src/kudu/client/client-internal.h
@@ -109,7 +109,8 @@ class KuduClient::Data {
bool has_range_partition_bounds,
bool has_range_specific_hash_schema,
bool has_immutable_column_schema,
- bool has_auto_incrementing_column);
+ bool has_auto_incrementing_column,
+ bool has_nested_columns);
static Status IsCreateTableInProgress(KuduClient* client,
master::TableIdentifierPB table,
@@ -137,7 +138,8 @@ class KuduClient::Data {
const MonoTime& deadline,
bool has_add_drop_partition,
bool adding_range_with_custom_hash_schema,
- bool has_immutable_column_schema);
+ bool has_immutable_column_schema,
+ bool has_nested_columns);
static Status IsAlterTableInProgress(KuduClient* client,
master::TableIdentifierPB table,
diff --git a/src/kudu/client/client.cc b/src/kudu/client/client.cc
index 5c28a401c..92d8a7ac5 100644
--- a/src/kudu/client/client.cc
+++ b/src/kudu/client/client.cc
@@ -31,7 +31,6 @@
#include <vector>
#include <glog/logging.h>
-#include <google/protobuf/stubs/common.h>
#include "kudu/client/callbacks.h"
#include "kudu/client/client-internal.h"
@@ -1125,7 +1124,6 @@ Status KuduTableCreator::Create() {
break;
}
}
- bool has_auto_incrementing_column =
data_->schema_->schema_->has_auto_incrementing();
if (data_->table_type_) {
req.set_table_type(*data_->table_type_);
@@ -1138,6 +1136,8 @@ Status KuduTableCreator::Create() {
deadline += data_->client_->default_admin_operation_timeout();
}
+ const bool has_auto_incrementing_column =
data_->schema_->schema_->has_auto_incrementing();
+ const bool has_nested_columns =
data_->schema_->schema_->has_nested_columns();
CreateTableResponsePB resp;
RETURN_NOT_OK_PREPEND(
data_->client_->data_->CreateTable(data_->client_,
@@ -1147,7 +1147,8 @@ Status KuduTableCreator::Create() {
!data_->range_partitions_.empty(),
has_range_with_custom_hash_schema,
has_immutable_column_schema,
- has_auto_incrementing_column),
+ has_auto_incrementing_column,
+ has_nested_columns),
Substitute("Error creating table $0 on the master", data_->table_name_));
// Spin until the table is fully created, if requested.
if (data_->wait_) {
@@ -1778,7 +1779,8 @@ Status KuduTableAlterer::Alter() {
data_->client_, req, &resp, deadline,
data_->has_alter_partitioning_steps,
data_->adding_range_with_custom_hash_schema,
- has_immutable_column_schema));
+ has_immutable_column_schema,
+ data_->has_nested_column_update_));
if (data_->has_alter_partitioning_steps) {
// If the table partitions change, clear the local meta cache so that the
diff --git a/src/kudu/client/service-features-test.cc
b/src/kudu/client/service-features-test.cc
new file mode 100644
index 000000000..acb878cc2
--- /dev/null
+++ b/src/kudu/client/service-features-test.cc
@@ -0,0 +1,150 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <memory>
+#include <string>
+#include <type_traits>
+#include <vector>
+
+#include <gflags/gflags_declare.h>
+#include <gtest/gtest.h>
+
+#include "kudu/client/client.h"
+#include "kudu/client/schema.h"
+#include "kudu/client/shared_ptr.h" // IWYU pragma: keep
+#include "kudu/common/common.pb.h"
+#include "kudu/common/schema.h"
+#include "kudu/mini-cluster/internal_mini_cluster.h"
+#include "kudu/util/status.h"
+#include "kudu/util/test_macros.h"
+#include "kudu/util/test_util.h"
+
+DECLARE_bool(master_support_1d_array_columns);
+
+using kudu::client::sp::shared_ptr;
+using kudu::cluster::InternalMiniCluster;
+using std::string;
+using std::unique_ptr;
+using std::vector;
+
+namespace kudu {
+namespace client {
+
+class ServiceFeaturesITest : public KuduTest {
+ public:
+ void SetUp() override {
+ // Set up the mini cluster
+ cluster_.reset(new InternalMiniCluster(env_, {}));
+ ASSERT_OK(cluster_->Start());
+ ASSERT_OK(cluster_->CreateClient(nullptr, &client_));
+ }
+
+ protected:
+ unique_ptr<InternalMiniCluster> cluster_;
+ shared_ptr<KuduClient> client_;
+};
+
+// This test scenario verifies that Kudu C++ client of this version properly
+// reports an error upon failed DDL requests involving tables with array type
+// columns. If such DDL requests are run against Kudu master that doesn't
+// support array type columns, and the behavior is as expected and the error
+// message output by the client is actionable.
+TEST_F(ServiceFeaturesITest, ArrayColumnSupportMaster) {
+ constexpr const char* const kTableName = "array_columns_support_m";
+ const Schema array_col_schema({
+ ColumnSchema("key", INT32),
+ ColumnSchemaBuilder().name("val").type(INT32).array(true).nullable(true)
+ }, 1);
+
+ // Make master not declaring its ARRAY_1D_COLUMN_TYPE feature.
+ FLAGS_master_support_1d_array_columns = false;
+
+ unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
+ KuduSchema schema(KuduSchema::FromSchema(array_col_schema));
+ table_creator->table_name(kTableName)
+ .schema(&schema)
+ .add_hash_partitions({ "key" }, 2)
+ .num_replicas(1);
+ const auto s = table_creator->Create();
+ ASSERT_TRUE(s.IsNotSupported()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(),
+ "Error creating table array_columns_support_m on the master: cluster "
+ "does not support CreateTable with feature(s) ARRAY_1D_COLUMN_TYPE");
+
+ // If master has ARRAY_1D_COLUMN_TYPE feature, it's possible to create tables
+ // with array columns in the system catalog.
+ FLAGS_master_support_1d_array_columns = true;
+ ASSERT_OK(table_creator->Create());
+
+ {
+ unique_ptr<KuduTableAlterer> alt(client_->NewTableAlterer(kTableName));
+ alt->AddColumn("c0")->NestedType(
+ KuduColumnSchema::KuduNestedTypeDescriptor(
+
KuduColumnSchema::KuduArrayTypeDescriptor(KuduColumnSchema::INT8)));
+ ASSERT_OK(alt->Alter());
+ }
+
+ // Below are unlikely sub-scenarios specific to disabling array type column
+ // support in a Kudu master (i.e. --setting
master_support_1d_array_columns=false)
+ // after a table with array column(s) has been created during prior run of
+ // the Kudu master while support for array type columns was enabled.
+ FLAGS_master_support_1d_array_columns = false;
+
+ // It's still possible to rename and drop the column, update the column's
+ // comment, etc. However, it's not possible to add a new array type column.
+ {
+ unique_ptr<KuduTableAlterer> alt(client_->NewTableAlterer(kTableName));
+ alt->AlterColumn("c0")->RenameTo("array0");
+ ASSERT_OK(alt->Alter());
+ }
+ {
+ unique_ptr<KuduTableAlterer> alt(client_->NewTableAlterer(kTableName));
+ alt->AlterColumn("array0")->Comment("comment");
+ ASSERT_OK(alt->Alter());
+ }
+ {
+ unique_ptr<KuduTableAlterer> alt(client_->NewTableAlterer(kTableName));
+ alt->DropColumn("array0");
+ ASSERT_OK(alt->Alter());
+ }
+ {
+ unique_ptr<KuduTableAlterer> alt(client_->NewTableAlterer(kTableName));
+ alt->AddColumn("c0")->NestedType(
+ KuduColumnSchema::KuduNestedTypeDescriptor(
+
KuduColumnSchema::KuduArrayTypeDescriptor(KuduColumnSchema::INT16)));
+ const auto s = alt->Alter();
+ ASSERT_TRUE(s.IsNotSupported()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(),
+ "cluster does not support AlterTable with feature(s)
ARRAY_1D_COLUMN_TYPE");
+ }
+ {
+ // It should be possible to open the table.
+ shared_ptr<KuduTable> table;
+ ASSERT_OK(client_->OpenTable(kTableName, &table));
+ }
+ // It should be possible to drop the table.
+ ASSERT_OK(client_->DeleteTable(kTableName));
+
+ {
+ vector<string> tables;
+ ASSERT_OK(client_->ListTables(&tables));
+ ASSERT_TRUE(tables.empty());
+ }
+}
+
+} // namespace client
+} // namespace kudu
diff --git a/src/kudu/client/table_alterer-internal.cc
b/src/kudu/client/table_alterer-internal.cc
index cd02bb61c..6ce8302b0 100644
--- a/src/kudu/client/table_alterer-internal.cc
+++ b/src/kudu/client/table_alterer-internal.cc
@@ -24,7 +24,6 @@
#include <utility>
#include <glog/logging.h>
-#include <google/protobuf/stubs/common.h>
#include "kudu/client/schema-internal.h"
#include "kudu/client/schema.h"
@@ -33,6 +32,7 @@
#include "kudu/common/row_operations.h"
#include "kudu/common/row_operations.pb.h"
#include "kudu/common/schema.h"
+#include "kudu/common/types.h"
#include "kudu/common/wire_protocol.h"
#include "kudu/master/master.pb.h"
@@ -46,6 +46,7 @@ KuduTableAlterer::Data::Data(KuduClient* client, string name)
: client_(client),
table_name_(std::move(name)),
wait_(true),
+ has_nested_column_update_(false),
schema_(nullptr) {
}
@@ -121,6 +122,9 @@ Status
KuduTableAlterer::Data::ToRequest(AlterTableRequestPB* req) {
ColumnSchemaToPB(*col.col_,
pb_step->mutable_add_column()->mutable_schema(),
SCHEMA_PB_WITHOUT_WRITE_DEFAULT);
+ if (col.col_->type_info()->type() == DataType::NESTED) {
+ has_nested_column_update_ = true;
+ }
break;
}
case AlterTableRequestPB::DROP_COLUMN:
diff --git a/src/kudu/client/table_alterer-internal.h
b/src/kudu/client/table_alterer-internal.h
index b186fa890..a715705c6 100644
--- a/src/kudu/client/table_alterer-internal.h
+++ b/src/kudu/client/table_alterer-internal.h
@@ -37,7 +37,7 @@ namespace client {
class KuduColumnSpec;
-class KuduTableAlterer::Data {
+class KuduTableAlterer::Data final {
public:
Data(KuduClient* client, std::string name);
~Data();
@@ -84,6 +84,9 @@ class KuduTableAlterer::Data {
// Set to true if a new range with custom hash schema is being added.
bool adding_range_with_custom_hash_schema = false;
+ // Set to true if a nested type column is being added or altered.
+ bool has_nested_column_update_ = false;
+
// Schema of add/drop range partition bound rows.
const Schema* schema_;
diff --git a/src/kudu/common/schema.cc b/src/kudu/common/schema.cc
index 9c4132f40..92443f654 100644
--- a/src/kudu/common/schema.cc
+++ b/src/kudu/common/schema.cc
@@ -228,6 +228,7 @@ void Schema::CopyFrom(const Schema& other) {
first_is_deleted_virtual_column_idx_ =
other.first_is_deleted_virtual_column_idx_;
has_nullables_ = other.has_nullables_;
+ has_nested_columns_ = other.has_nested_columns_;
auto_incrementing_col_idx_ = other.auto_incrementing_col_idx_;
}
@@ -241,6 +242,7 @@ Schema::Schema(Schema&& other) noexcept
id_to_index_(std::move(other.id_to_index_)),
first_is_deleted_virtual_column_idx_(other.first_is_deleted_virtual_column_idx_),
has_nullables_(other.has_nullables_),
+ has_nested_columns_(other.has_nested_columns_),
auto_incrementing_col_idx_(other.auto_incrementing_col_idx_) {
}
@@ -254,6 +256,7 @@ Schema& Schema::operator=(Schema&& other) noexcept {
id_to_index_ = std::move(other.id_to_index_);
first_is_deleted_virtual_column_idx_ =
other.first_is_deleted_virtual_column_idx_;
has_nullables_ = other.has_nullables_;
+ has_nested_columns_ = other.has_nested_columns_;
name_to_index_ = std::move(other.name_to_index_);
auto_incrementing_col_idx_ = other.auto_incrementing_col_idx_;
}
@@ -299,6 +302,7 @@ Status Schema::Reset(vector<ColumnSchema> cols,
}
}
+ has_nested_columns_ = false;
auto_incrementing_col_idx_ = auto_incrementing_col_idx;
// Calculate the offset of each column in the row format.
col_offsets_.clear();
@@ -325,6 +329,9 @@ Status Schema::Reset(vector<ColumnSchema> cols,
col_offsets_.push_back(off);
off += col.type_info()->size();
+ if (col.type_info()->type() == DataType::NESTED) {
+ has_nested_columns_ = true;
+ }
}
// Add an extra element on the end for the total
diff --git a/src/kudu/common/schema.h b/src/kudu/common/schema.h
index be61345fe..14b744d78 100644
--- a/src/kudu/common/schema.h
+++ b/src/kudu/common/schema.h
@@ -886,6 +886,16 @@ class Schema {
return has_nullables_;
}
+ // Returns true if the schema contains at least one nested column.
+ //
+ // NOTE: As of time of writing, a nested column can only be of 1D array type.
+ // When more nested types are supported, the 'has_nested_columns' parameter
+ // needs to become an integer where different bits stands for particular
+ // features required at the server-side.
+ bool has_nested_columns() const {
+ return has_nested_columns_;
+ }
+
// Returns true if the specified column (by name) is a key
bool is_key_column(const StringPiece col_name) const {
return is_key_column(find_column(col_name));
@@ -1282,6 +1292,9 @@ class Schema {
// Cached indicator whether any columns are nullable.
bool has_nullables_;
+ // Cached indicator whether any columns are of array type.
+ bool has_nested_columns_ = false;
+
// Cached index of the auto-incrementing column, or kColumnNotFound if no
// such column exists in the schema.
int auto_incrementing_col_idx_;
diff --git a/src/kudu/master/master.proto b/src/kudu/master/master.proto
index f8ff85d3e..12b74f4d6 100644
--- a/src/kudu/master/master.proto
+++ b/src/kudu/master/master.proto
@@ -1214,6 +1214,9 @@ enum MasterFeatures {
IMMUTABLE_COLUMN_ATTRIBUTE = 10;
// Whether master supports auto incrementing column.
AUTO_INCREMENTING_COLUMN = 11;
+ // Whether the system catalog supports tables with one-dimensional array
+ // type columns.
+ ARRAY_1D_COLUMN_TYPE = 12;
}
service MasterService {
diff --git a/src/kudu/master/master_service.cc
b/src/kudu/master/master_service.cc
index f6dbe55cf..e2af71714 100644
--- a/src/kudu/master/master_service.cc
+++ b/src/kudu/master/master_service.cc
@@ -125,6 +125,12 @@ TAG_FLAG(master_support_auto_incrementing_column, unsafe);
TAG_FLAG(master_support_auto_incrementing_column, experimental);
TAG_FLAG(master_support_auto_incrementing_column, runtime);
+DEFINE_bool(master_support_1d_array_columns, true,
+ "Whether the system catalog supports creating tables with "
+ "one-dimensional columns");
+TAG_FLAG(master_support_1d_array_columns, hidden);
+TAG_FLAG(master_support_1d_array_columns, runtime);
+
using google::protobuf::Message;
using kudu::consensus::ReplicaManagementInfoPB;
using kudu::pb_util::SecureDebugString;
@@ -991,6 +997,8 @@ bool MasterServiceImpl::SupportsFeature(uint32_t feature)
const {
return FLAGS_master_support_immutable_column_attribute;
case MasterFeatures::AUTO_INCREMENTING_COLUMN:
return FLAGS_master_support_auto_incrementing_column;
+ case MasterFeatures::ARRAY_1D_COLUMN_TYPE:
+ return FLAGS_master_support_1d_array_columns;
default:
return false;
}
