KUDU-2645: schema: Validate that defaults are provided for virtual co…

…lumns

Virtual columns must not be nullable and must have read defaults.
Instead of crashing when we encounter violations of these requirements,
return a bad status code when mapping such a projection onto a tablet
schema.

TODO(KUDU-2692): Consider removing the requirement to specify a read
default on a virtual column by defining a built-in read default for each
virtual column type, such as a default of false for IS_DELETED.

Change-Id: I9144390f9ed02789a1ea161277b5f1567cfc2ffe
Reviewed-on: http://gerrit.cloudera.org:8080/12416
Reviewed-by: Adar Dembo <[email protected]>
Tested-by: Kudu Jenkins

src/kudu/common/schema-test.cc

@@ -448,8 +448,10 @@ TEST_F(TestSchema, TestGetMappedReadProjection) {
                        { ColumnId(0),
                          ColumnId(1) },
                        1);
+  const bool kReadDefault = false;
   Schema projection({ ColumnSchema("key", STRING),
-                      ColumnSchema("deleted", IS_DELETED) },
+                      ColumnSchema("deleted", IS_DELETED,
+                                   /*is_nullable=*/false, /*read_default=*/&kReadDefault) },
                     1);
 
   Schema mapped;
@@ -470,6 +472,26 @@ TEST_F(TestSchema, TestGetMappedReadProjection) {
   ASSERT_EQ("deleted", mapped.column(1).name());
   ASSERT_GT(mapped.column_id(1), tablet_schema.column_id(1));
   ASSERT_GT(mapped.max_col_id(), tablet_schema.max_col_id());
+
+  // Ensure that virtual columns that are nullable or that do not have read
+  // defaults are rejected.
+  Schema nullable_projection({ ColumnSchema("key", STRING),
+                               ColumnSchema("deleted", IS_DELETED,
+                                            /*is_nullable=*/true,
+                                            /*read_default=*/&kReadDefault) },
+                          1);
+  Status s = tablet_schema.GetMappedReadProjection(nullable_projection, &mapped);
+  ASSERT_FALSE(s.ok());
+  ASSERT_STR_CONTAINS(s.ToString(), "must not be nullable");
+
+  Schema no_default_projection({ ColumnSchema("key", STRING),
+                                 ColumnSchema("deleted", IS_DELETED,
+                                              /*is_nullable=*/false,
+                                              /*read_default=*/nullptr) },
+                               1);
+  s = tablet_schema.GetMappedReadProjection(no_default_projection, &mapped);
+  ASSERT_FALSE(s.ok());
+  ASSERT_STR_CONTAINS(s.ToString(), "must have a default value for read");
 }
 
 // Test that the schema can be used to compare and stringify rows.

src/kudu/common/schema.cc

@@ -401,6 +401,15 @@ Status Schema::GetMappedReadProjection(const Schema& projection,
     int index = find_column(col.name());
     if (col.type_info()->is_virtual()) {
       DCHECK_EQ(kColumnNotFound, index) << "virtual column not expected in tablet schema";
+      if (col.is_nullable()) {
+        return Status::InvalidArgument(Substitute("Virtual column $0 $1 must not be nullable",
+                                                  col.name(), col.TypeToString()));
+      }
+      if (!col.has_read_default()) {
+        return Status::InvalidArgument(Substitute("Virtual column $0 $1 must "
+                                                  "have a default value for read",
+                                                  col.name(), col.TypeToString()));
+      }
       mapped_cols.push_back(col);
       // Generate a "fake" column id for virtual columns.
       mapped_ids.emplace_back(++proj_max_col_id);

src/kudu/common/schema.h

@@ -904,6 +904,7 @@ class Schema {
       if (col.type_info()->type() == IS_DELETED) {
         // Enforce some properties on the virtual column that simplify our
         // implementation.
+        // TODO(KUDU-2692): Consider removing these requirements.
         DCHECK(!col.is_nullable());
         DCHECK(col.has_read_default());