kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From danburk...@apache.org
Subject kudu git commit: KUDU-861 Support changing default, storage attributes
Date Thu, 08 Dec 2016 00:43:11 GMT
Repository: kudu
Updated Branches:
  refs/heads/master b4d1fac80 -> e16a541ae


KUDU-861 Support changing default, storage attributes

This patch adds support for adding, changing, or removing column defaults
and changing the storage attributes of a column. Changes to a column are
encoded as a ColumnSchemaDelta, which can be merged with a ColumnSchema
to change it.

Changing type and nullability of a column is still unsupported.

No failures in 100+ iterations of alter_table-randomized-test.

I also ran into an issue with altering RLE columns:
1. Add an RLE-encoded column to a table
2. Alter the column
3. Scan the column
3 will cause a check failure on scanning to pos 0 of an empty RLE
block. Test and fix included.

Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Reviewed-on: http://gerrit.cloudera.org:8080/4310
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <danburkert@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/e16a541a
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/e16a541a
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/e16a541a

Branch: refs/heads/master
Commit: e16a541aedc171eadb738e969a48852ba9ed2909
Parents: b4d1fac
Author: Will Berkeley <wdberkeley@gmail.com>
Authored: Sun Sep 4 13:50:04 2016 -0400
Committer: Dan Burkert <danburkert@apache.org>
Committed: Thu Dec 8 00:42:45 2016 +0000

----------------------------------------------------------------------
 .../apache/kudu/client/AlterTableOptions.java   | 108 +++++++++-
 .../org/apache/kudu/client/ProtobufHelper.java  |  41 +++-
 .../org/apache/kudu/client/TestKuduTable.java   | 100 +++++++++-
 src/kudu/cfile/rle_block.h                      |   2 +-
 src/kudu/client/client-test.cc                  | 195 +++++++++++++++----
 src/kudu/client/schema.cc                       |  54 ++++-
 src/kudu/client/schema.h                        |   5 +
 src/kudu/client/table_alterer-internal.cc       |  32 +--
 src/kudu/client/value-internal.h                |   4 +
 src/kudu/client/value.cc                        |  18 ++
 src/kudu/common/common.proto                    |  12 ++
 src/kudu/common/schema.cc                       |  85 ++++++--
 src/kudu/common/schema.h                        |  41 +++-
 src/kudu/common/wire_protocol.cc                |  48 +++++
 src/kudu/common/wire_protocol.h                 |   8 +
 .../alter_table-randomized-test.cc              | 109 ++++++++++-
 src/kudu/integration-tests/alter_table-test.cc  | 110 +++++++++++
 src/kudu/master/catalog_manager.cc              |  27 ++-
 src/kudu/master/master.proto                    |   9 +-
 19 files changed, 914 insertions(+), 94 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
----------------------------------------------------------------------
diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java b/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
index 31617aa..3827ed0 100644
--- a/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
+++ b/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
@@ -17,11 +17,15 @@
 
 package org.apache.kudu.client;
 
+import static org.apache.kudu.ColumnSchema.CompressionAlgorithm;
+import static org.apache.kudu.ColumnSchema.Encoding;
 import static org.apache.kudu.master.Master.AlterTableRequestPB;
 
 import com.google.common.base.Preconditions;
+import com.google.protobuf.ByteString;
 
 import org.apache.kudu.ColumnSchema;
+import org.apache.kudu.Common;
 import org.apache.kudu.Type;
 import org.apache.kudu.annotations.InterfaceAudience;
 import org.apache.kudu.annotations.InterfaceStability;
@@ -121,9 +125,107 @@ public class AlterTableOptions {
    */
   public AlterTableOptions renameColumn(String oldName, String newName) {
     AlterTableRequestPB.Step.Builder step = pb.addAlterSchemaStepsBuilder();
-    step.setType(AlterTableRequestPB.StepType.RENAME_COLUMN);
-    step.setRenameColumn(AlterTableRequestPB.RenameColumn.newBuilder().setOldName(oldName)
-        .setNewName(newName));
+    step.setType(AlterTableRequestPB.StepType.ALTER_COLUMN);
+    AlterTableRequestPB.AlterColumn.Builder alterBuilder =
+        AlterTableRequestPB.AlterColumn.newBuilder();
+    alterBuilder.setDelta(
+        Common.ColumnSchemaDeltaPB.newBuilder().setName(oldName).setNewName(newName));
+    step.setAlterColumn(alterBuilder);
+    return this;
+  }
+
+  /**
+   * Remove the default value for a column.
+   * @param name name of the column
+   * @return this instance
+   */
+  public AlterTableOptions removeDefault(String name) {
+    AlterTableRequestPB.Step.Builder step = pb.addAlterSchemaStepsBuilder();
+    step.setType(AlterTableRequestPB.StepType.ALTER_COLUMN);
+    AlterTableRequestPB.AlterColumn.Builder alterBuilder =
+        AlterTableRequestPB.AlterColumn.newBuilder();
+    alterBuilder.setDelta(
+        Common.ColumnSchemaDeltaPB.newBuilder().setName(name).setRemoveDefault(true));
+    step.setAlterColumn(alterBuilder);
+    return this;
+  }
+
+  /**
+   * Change the default value for a column. `newDefault` must not be null or
+   * else throws {@link IllegalArgumentException}.
+   * @param name name of the column
+   * @param newDefault the new default value
+   * @return this instance
+   */
+  public AlterTableOptions changeDefault(String name, Object newDefault) {
+    if (newDefault == null) {
+      throw new IllegalArgumentException("newDefault cannot be null: " +
+          "use removeDefault to clear a default value");
+    }
+
+    ByteString defaultByteString = ProtobufHelper.objectToByteStringNoType(name, newDefault);
+    AlterTableRequestPB.Step.Builder step = pb.addAlterSchemaStepsBuilder();
+    step.setType(AlterTableRequestPB.StepType.ALTER_COLUMN);
+    AlterTableRequestPB.AlterColumn.Builder alterBuilder =
+        AlterTableRequestPB.AlterColumn.newBuilder();
+    alterBuilder.setDelta(
+        Common.ColumnSchemaDeltaPB.newBuilder().setName(name)
+            .setDefaultValue(defaultByteString));
+    step.setAlterColumn(alterBuilder);
+    return this;
+  }
+
+  /**
+   * Change the block size of a column's storage. A nonpositive value indicates
+   * a server-side default.
+   * @param name name of the column
+   * @param blockSize the new block size
+   * @return this instance
+   */
+  public AlterTableOptions changeDesiredBlockSize(String name, int blockSize) {
+    AlterTableRequestPB.Step.Builder step = pb.addAlterSchemaStepsBuilder();
+    step.setType(AlterTableRequestPB.StepType.ALTER_COLUMN);
+    AlterTableRequestPB.AlterColumn.Builder alterBuilder =
+        AlterTableRequestPB.AlterColumn.newBuilder();
+    alterBuilder.setDelta(
+        Common.ColumnSchemaDeltaPB.newBuilder().setName(name).setBlockSize(blockSize));
+    step.setAlterColumn(alterBuilder);
+    return this;
+  }
+
+  /**
+   * Change the encoding used for a column.
+   * @param name name of the column
+   * @param encoding the new encoding
+   * @return this instance
+   */
+  public AlterTableOptions changeEncoding(String name, Encoding encoding) {
+    AlterTableRequestPB.Step.Builder step = pb.addAlterSchemaStepsBuilder();
+    step.setType(AlterTableRequestPB.StepType.ALTER_COLUMN);
+    AlterTableRequestPB.AlterColumn.Builder alterBuilder =
+        AlterTableRequestPB.AlterColumn.newBuilder();
+    alterBuilder.setDelta(
+        Common.ColumnSchemaDeltaPB.newBuilder().setName(name)
+            .setEncoding(encoding.getInternalPbType()));
+    step.setAlterColumn(alterBuilder);
+    return this;
+  }
+
+  /**
+   * Change the compression used for a column.
+   * @param name the name of the column
+   * @param ca the new compression algorithm
+   * @return this instance
+   */
+  public AlterTableOptions changeCompressionAlgorithm(String name, CompressionAlgorithm ca) {
+    AlterTableRequestPB.Step.Builder step = pb.addAlterSchemaStepsBuilder();
+    step.setType(AlterTableRequestPB.StepType.ALTER_COLUMN);
+    AlterTableRequestPB.AlterColumn.Builder alterBuilder =
+        AlterTableRequestPB.AlterColumn.newBuilder();
+    alterBuilder.setDelta(
+        Common.ColumnSchemaDeltaPB.newBuilder().setName(name)
+            .setCompression(ca.getInternalPbType()));
+    step.setAlterColumn(alterBuilder);
     return this;
   }
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
----------------------------------------------------------------------
diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java b/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
index 5831e25..76e6be6 100644
--- a/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
+++ b/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
@@ -17,6 +17,7 @@
 
 package org.apache.kudu.client;
 
+import java.nio.ByteBuffer;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -236,9 +237,47 @@ public class ProtobufHelper {
   }
 
   /**
+   * Serializes an object based on its Java type. Used for Alter Column
+   * operations where the column's type is not available. `value` must be
+   * a Kudu-compatible type or else throw {@link IllegalArgumentException}.
+   *
+   * @param colName the name of the column (for the error message)
+   * @param value the value to serialize
+   * @return the serialized object
+   */
+  protected static ByteString objectToByteStringNoType(String colName, Object value) {
+    byte[] bytes;
+    if (value instanceof Boolean) {
+      bytes = Bytes.fromBoolean((Boolean) value);
+    } else if (value instanceof Byte) {
+      bytes = new byte[] {(Byte) value};
+    } else if (value instanceof Short) {
+      bytes = Bytes.fromShort((Short) value);
+    } else if (value instanceof Integer) {
+      bytes = Bytes.fromInt((Integer) value);
+    } else if (value instanceof Long) {
+      bytes = Bytes.fromLong((Long) value);
+    } else if (value instanceof String) {
+      bytes = ((String) value).getBytes(Charsets.UTF_8);
+    } else if (value instanceof byte[]) {
+      bytes = (byte[]) value;
+    } else if (value instanceof ByteBuffer) {
+      bytes = ((ByteBuffer) value).array();
+    } else if (value instanceof Float) {
+      bytes = Bytes.fromFloat((Float) value);
+    } else if (value instanceof Double) {
+      bytes = Bytes.fromDouble((Double) value);
+    } else {
+      throw new IllegalArgumentException("The default value provided for " +
+          "column " + colName + " is of class " + value.getClass().getName() +
+          " which does not map to a supported Kudu type");
+    }
+    return ZeroCopyLiteralByteString.wrap(bytes);
+  }
+
+  /**
    * Convert a {@link com.google.common.net.HostAndPort} to
    *     {@link org.apache.kudu.Common.HostPortPB}
-   * protobuf message for serialization.
    * @param hostAndPort The host and port object. Both host and port must be specified.
    * @return An initialized HostPortPB object.
    */

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
----------------------------------------------------------------------
diff --git a/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java b/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
index 09ef520..8fa9ad0 100644
--- a/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
+++ b/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
@@ -16,7 +16,12 @@
 // under the License.
 package org.apache.kudu.client;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -47,6 +52,98 @@ public class TestKuduTable extends BaseKuduTest {
     BaseKuduTest.setUpBeforeClass();
   }
 
+  @Test
+  public void testAlterColumn() throws Exception {
+    // Used a simplified schema because basicSchema has extra columns that make the asserts verbose.
+    String tableName = name.getMethodName() + System.currentTimeMillis();
+    List<ColumnSchema> columns = ImmutableList.of(
+        new ColumnSchema.ColumnSchemaBuilder("key", Type.INT32).key(true).build(),
+        new ColumnSchema.ColumnSchemaBuilder("value", Type.STRING)
+            .nullable(true)
+            .desiredBlockSize(4096)
+            .encoding(ColumnSchema.Encoding.PLAIN_ENCODING)
+            .compressionAlgorithm(ColumnSchema.CompressionAlgorithm.NO_COMPRESSION)
+            .build());
+    KuduTable table = createTable(tableName, new Schema(columns), getBasicCreateTableOptions());
+    KuduSession session = syncClient.newSession();
+    // Insert a row before a default is defined and check the value is NULL.
+    insertDefaultRow(table, session, 0);
+    List<String> rows = scanTableToStrings(table);
+    assertEquals("wrong number of rows", 1, rows.size());
+    assertEquals("wrong row", "INT32 key=0, STRING value=NULL", rows.get(0));
+
+    // Add a default, checking new rows see the new default and old rows remain the same.
+    AlterTableOptions ato = new AlterTableOptions().changeDefault("value", "pizza");
+    submitAlterAndCheck(ato, tableName);
+
+    insertDefaultRow(table, session, 1);
+    rows = scanTableToStrings(table);
+    assertEquals("wrong number of rows", 2, rows.size());
+    assertEquals("wrong row", "INT32 key=0, STRING value=NULL", rows.get(0));
+    assertEquals("wrong row", "INT32 key=1, STRING value=pizza", rows.get(1));
+
+    // Change the default, checking new rows see the new default and old rows remain the same.
+    ato = new AlterTableOptions().changeDefault("value", "taco");
+    submitAlterAndCheck(ato, tableName);
+
+    insertDefaultRow(table, session, 2);
+
+    rows = scanTableToStrings(table);
+    assertEquals("wrong number of rows", 3, rows.size());
+    assertEquals("wrong row", "INT32 key=0, STRING value=NULL", rows.get(0));
+    assertEquals("wrong row", "INT32 key=1, STRING value=pizza", rows.get(1));
+    assertEquals("wrong row", "INT32 key=2, STRING value=taco", rows.get(2));
+
+    // Remove the default, checking that new rows default to NULL and old rows remain the same.
+    ato = new AlterTableOptions().removeDefault("value");
+    submitAlterAndCheck(ato, tableName);
+
+    insertDefaultRow(table, session, 3);
+
+    rows = scanTableToStrings(table);
+    assertEquals("wrong number of rows", 4, rows.size());
+    assertEquals("wrong row", "INT32 key=0, STRING value=NULL", rows.get(0));
+    assertEquals("wrong row", "INT32 key=1, STRING value=pizza", rows.get(1));
+    assertEquals("wrong row", "INT32 key=2, STRING value=taco", rows.get(2));
+    assertEquals("wrong row", "INT32 key=3, STRING value=NULL", rows.get(3));
+
+    // Change the column storage attributes.
+    assertEquals("wrong block size",
+        4096,
+        table.getSchema().getColumn("value").getDesiredBlockSize());
+    assertEquals("wrong encoding",
+        ColumnSchema.Encoding.PLAIN_ENCODING,
+        table.getSchema().getColumn("value").getEncoding());
+    assertEquals("wrong compression algorithm",
+        ColumnSchema.CompressionAlgorithm.NO_COMPRESSION,
+        table.getSchema().getColumn("value").getCompressionAlgorithm());
+
+    ato = new AlterTableOptions().changeDesiredBlockSize("value", 8192)
+        .changeEncoding("value", ColumnSchema.Encoding.DICT_ENCODING)
+        .changeCompressionAlgorithm("value", ColumnSchema.CompressionAlgorithm.SNAPPY);
+    submitAlterAndCheck(ato, tableName);
+
+    KuduTable reopenedTable = syncClient.openTable(tableName);
+    assertEquals("wrong block size post alter",
+        8192,
+        reopenedTable.getSchema().getColumn("value").getDesiredBlockSize());
+    assertEquals("wrong encoding post alter",
+        ColumnSchema.Encoding.DICT_ENCODING,
+        reopenedTable.getSchema().getColumn("value").getEncoding());
+    assertEquals("wrong compression algorithm post alter",
+        ColumnSchema.CompressionAlgorithm.SNAPPY,
+        reopenedTable.getSchema().getColumn("value").getCompressionAlgorithm());
+  }
+
+  private void insertDefaultRow(KuduTable table, KuduSession session, int key)
+      throws Exception {
+    Insert insert = table.newInsert();
+    PartialRow row = insert.getRow();
+    row.addInt("key", key);
+    // Omit value.
+    session.apply(insert);
+  }
+
   @Test(timeout = 100000)
   public void testAlterTable() throws Exception {
     String tableName = name.getMethodName() + System.currentTimeMillis();
@@ -83,7 +180,6 @@ public class TestKuduTable extends BaseKuduTest {
               (System.currentTimeMillis() * 1000));
       submitAlterAndCheck(ato, tableName);
 
-
       // Try altering a table that doesn't exist.
       String nonExistingTableName = "table_does_not_exist";
       try {

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/cfile/rle_block.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/rle_block.h b/src/kudu/cfile/rle_block.h
index c7702f1..3cecf97 100644
--- a/src/kudu/cfile/rle_block.h
+++ b/src/kudu/cfile/rle_block.h
@@ -330,7 +330,7 @@ class RleIntBlockDecoder final : public BlockDecoder {
 
   virtual void SeekToPositionInBlock(uint pos) OVERRIDE {
     CHECK(parsed_) << "Must call ParseHeader()";
-    CHECK_LT(pos, num_elems_)
+    CHECK(pos < num_elems_ || (num_elems_ == 0 && pos == 0))
         << "Tried to seek to " << pos << " which is >= number of elements ("
         << num_elems_ << ") in the block!.";
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/client/client-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc
index 4e7aaa5..1fc480f 100644
--- a/src/kudu/client/client-test.cc
+++ b/src/kudu/client/client-test.cc
@@ -350,7 +350,7 @@ class ClientTest : public KuduTest {
   void DoTestScanResourceMetrics() {
     KuduScanner scanner(client_table_.get());
     string tablet_id = GetFirstTabletId(client_table_.get());
-    // flush to ensure we scan disk later
+    // Flush to ensure we scan disk later.
     FlushTablet(tablet_id);
     ASSERT_OK(scanner.SetProjectedColumns({ "key" }));
     LOG_TIMING(INFO, "Scanning disk with no predicates") {
@@ -712,8 +712,8 @@ TEST_F(ClientTest, TestScanAtSnapshot) {
   ASSERT_NO_FATAL_FAILURE(InsertTestRows(client_table_.get(),
                                          half_the_rows));
 
-  // get the time from the server and transform to micros disregarding any
-  // logical values (we shouldn't have any with a single server anyway);
+  // Get the time from the server and transform to micros, disregarding any
+  // logical values (we shouldn't have any with a single server anyway).
   int64_t ts = server::HybridClock::GetPhysicalValueMicros(
       cluster_->mini_tablet_server(0)->server()->clock()->Now());
 
@@ -1269,7 +1269,7 @@ TEST_F(ClientTest, TestNonCoveringRangePartitions) {
 
   // Scans
 
-  { // full table scan
+  { // Full table scan
     vector<string> rows;
     KuduScanner scanner(table.get());
     ASSERT_OK(scanner.SetFaultTolerant());
@@ -2945,7 +2945,7 @@ TEST_F(ClientTest, TestMutateDeletedRow) {
   Status s = session->Flush();
   ASSERT_FALSE(s.ok());
   ASSERT_STR_CONTAINS(s.ToString(), "Some errors occurred");
-  // verify error
+  // Verify error
   gscoped_ptr<KuduError> error = GetSingleErrorFromSession(session.get());
   ASSERT_EQ(error->failed_op().ToString(),
             "UPDATE int32 key=1, int32 int_val=2");
@@ -2957,7 +2957,7 @@ TEST_F(ClientTest, TestMutateDeletedRow) {
   s = session->Flush();
   ASSERT_FALSE(s.ok());
   ASSERT_STR_CONTAINS(s.ToString(), "Some errors occurred");
-  // verify error
+  // Verify error
   error = GetSingleErrorFromSession(session.get());
   ASSERT_EQ(error->failed_op().ToString(),
             "DELETE int32 key=1");
@@ -2975,7 +2975,7 @@ TEST_F(ClientTest, TestMutateNonexistentRow) {
   Status s = session->Flush();
   ASSERT_FALSE(s.ok());
   ASSERT_STR_CONTAINS(s.ToString(), "Some errors occurred");
-  // verify error
+  // Verify error
   gscoped_ptr<KuduError> error = GetSingleErrorFromSession(session.get());
   ASSERT_EQ(error->failed_op().ToString(),
             "UPDATE int32 key=1, int32 int_val=2");
@@ -2987,7 +2987,7 @@ TEST_F(ClientTest, TestMutateNonexistentRow) {
   s = session->Flush();
   ASSERT_FALSE(s.ok());
   ASSERT_STR_CONTAINS(s.ToString(), "Some errors occurred");
-  // verify error
+  // Verify error
   error = GetSingleErrorFromSession(session.get());
   ASSERT_EQ(error->failed_op().ToString(),
             "DELETE int32 key=1");
@@ -3113,7 +3113,39 @@ TEST_F(ClientTest, TestWriteWithBadSchema) {
 TEST_F(ClientTest, TestBasicAlterOperations) {
   const vector<string> kBadNames = {"", string(1000, 'x')};
 
-  // test that having no steps throws an error
+  // Test that renaming a column to an invalid name throws an error.
+  for (const string& bad_name : kBadNames) {
+    gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("int_val")->RenameTo(bad_name);
+    Status s = table_alterer->Alter();
+    ASSERT_TRUE(s.IsInvalidArgument());
+    ASSERT_STR_CONTAINS(s.ToString(), "invalid column name");
+  }
+
+  // Test that renaming a table to an invalid name throws an error.
+  for (const string& bad_name : kBadNames) {
+    gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->RenameTo(bad_name);
+    Status s = table_alterer->Alter();
+    ASSERT_TRUE(s.IsInvalidArgument());
+    ASSERT_STR_CONTAINS(s.ToString(), "invalid table name");
+  }
+
+  // Test trying to add columns to a table such that it exceeds the permitted
+  // maximum.
+  {
+    gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    for (int i = 0; i < 1000; i++) {
+    table_alterer->AddColumn(Substitute("c$0", i))->Type(KuduColumnSchema::INT32);
+    }
+    Status s = table_alterer->Alter();
+    ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+    ASSERT_STR_CONTAINS(s.ToString(),
+    "number of columns 1004 is greater than the "
+    "permitted maximum 300");
+  }
+
+  // Having no steps should throw an error.
   {
     gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
     Status s = table_alterer->Alter();
@@ -3121,7 +3153,7 @@ TEST_F(ClientTest, TestBasicAlterOperations) {
     ASSERT_STR_CONTAINS(s.ToString(), "No alter steps provided");
   }
 
-  // test that adding a non-nullable column with no default value throws an error
+  // Adding a non-nullable column with no default value should throw an error.
   {
     gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
     table_alterer->AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull();
@@ -3130,26 +3162,26 @@ TEST_F(ClientTest, TestBasicAlterOperations) {
     ASSERT_STR_CONTAINS(s.ToString(), "column `key`: NOT NULL columns must have a default");
   }
 
-  // test that remove key should throws an error
+  // Removing a key should throw an error.
   {
     gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
     Status s = table_alterer
       ->DropColumn("key")
       ->Alter();
     ASSERT_TRUE(s.IsInvalidArgument());
-    ASSERT_STR_CONTAINS(s.ToString(), "cannot remove a key column");
+    ASSERT_STR_CONTAINS(s.ToString(), "cannot remove a key column: key");
   }
 
-  // test that renaming a key should throws an error
+  // Renaming a key should throw an error.
   {
     gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
     table_alterer->AlterColumn("key")->RenameTo("key2");
     Status s = table_alterer->Alter();
     ASSERT_TRUE(s.IsInvalidArgument());
-    ASSERT_STR_CONTAINS(s.ToString(), "cannot rename a key column");
+    ASSERT_STR_CONTAINS(s.ToString(), "cannot alter a key column: key");
   }
 
-  // test that renaming to an already-existing name throws an error
+  // Renaming a column to an already-existing name should throw an error.
   {
     gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
     table_alterer->AlterColumn("int_val")->RenameTo("string_val");
@@ -3158,36 +3190,38 @@ TEST_F(ClientTest, TestBasicAlterOperations) {
     ASSERT_STR_CONTAINS(s.ToString(), "The column already exists: string_val");
   }
 
-  // Test that renaming a column to an invalid name throws an error.
-  for (const string& bad_name : kBadNames) {
+  // Altering a column but specifying no alterations should throw an error.
+  {
     gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
-    table_alterer->AlterColumn("int_val")->RenameTo(bad_name);
+    table_alterer->AlterColumn("string_val");
     Status s = table_alterer->Alter();
     ASSERT_TRUE(s.IsInvalidArgument());
-    ASSERT_STR_CONTAINS(s.ToString(), "invalid column name");
+    ASSERT_STR_CONTAINS(s.ToString(), "no alter operation specified: string_val");
   }
 
-  // Test that renaming a table to an invalid name throws an error.
-  for (const string& bad_name : kBadNames) {
+  // Trying to change type or nullability of a column is an error.
+  {
     gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
-    table_alterer->RenameTo(bad_name);
+    table_alterer->AlterColumn("string_val")->Type(KuduColumnSchema::STRING);
     Status s = table_alterer->Alter();
-    ASSERT_TRUE(s.IsInvalidArgument());
-    ASSERT_STR_CONTAINS(s.ToString(), "invalid table name");
+    ASSERT_TRUE(s.IsNotSupported());
+    ASSERT_STR_CONTAINS(s.ToString(), "unsupported alter operation: string_val");
   }
 
-  // Test trying to add columns to a table such that it exceeds the permitted
-  // maximum.
   {
     gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
-    for (int i = 0; i < 1000; i++) {
-      table_alterer->AddColumn(Substitute("c$0", i))->Type(KuduColumnSchema::INT32);
-    }
+    table_alterer->AlterColumn("string_val")->Nullable();
     Status s = table_alterer->Alter();
-    ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
-    ASSERT_STR_CONTAINS(s.ToString(),
-                        "number of columns 1004 is greater than the "
-                        "permitted maximum 300");
+    ASSERT_TRUE(s.IsNotSupported());
+    ASSERT_STR_CONTAINS(s.ToString(), "unsupported alter operation: string_val");
+  }
+
+  {
+    gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("string_val")->NotNull();
+    Status s = table_alterer->Alter();
+    ASSERT_TRUE(s.IsNotSupported());
+    ASSERT_STR_CONTAINS(s.ToString(), "unsupported alter operation: string_val");
   }
 
   // Need a tablet peer for the next set of tests.
@@ -3204,8 +3238,7 @@ TEST_F(ClientTest, TestBasicAlterOperations) {
     ASSERT_EQ(1, tablet_peer->tablet()->metadata()->schema_version());
   }
 
-  // test that specifying an encoding incompatible with the column's
-  // type throws an error
+  // Specifying an encoding incompatible with the column's type is an error.
   {
     gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
     table_alterer->AddColumn("new_string_val")->Type(KuduColumnSchema::STRING)
@@ -3224,13 +3257,103 @@ TEST_F(ClientTest, TestBasicAlterOperations) {
     ASSERT_EQ(2, tablet_peer->tablet()->metadata()->schema_version());
   }
 
+  // Test changing a default value for a binary column.
+  // There are separate tests for fixed- and variable-length types because
+  // the implementations differ a little
+  {
+    gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("string_val")
+        ->Default(KuduValue::CopyString("hello!"));
+    ASSERT_OK(table_alterer->Alter());
+    ASSERT_EQ(3, tablet_peer->tablet()->metadata()->schema_version());
+    Schema schema = tablet_peer->tablet()->metadata()->schema();
+    ColumnSchema col_schema = schema.column(schema.find_column("string_val"));
+    ASSERT_FALSE(col_schema.has_read_default());
+    ASSERT_TRUE(col_schema.has_write_default());
+    ASSERT_EQ("hello!", *reinterpret_cast<const Slice*>(col_schema.write_default_value()));
+  }
+
+  // Change a default value for an integer column.
+  {
+    gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("non_null_with_default")
+        ->Default(KuduValue::FromInt(54321));
+    ASSERT_OK(table_alterer->Alter());
+    ASSERT_EQ(4, tablet_peer->tablet()->metadata()->schema_version());
+    Schema schema = tablet_peer->tablet()->metadata()->schema();
+    ColumnSchema col_schema = schema.column(schema.find_column("non_null_with_default"));
+    ASSERT_TRUE(col_schema.has_read_default()); // Started with a default
+    ASSERT_TRUE(col_schema.has_write_default());
+    ASSERT_EQ(54321, *reinterpret_cast<const int32_t*>(col_schema.write_default_value()));
+  }
+
+  // Clear a default value from a nullable column
+  {
+    gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("string_val")
+        ->RemoveDefault();
+    ASSERT_OK(table_alterer->Alter());
+    ASSERT_EQ(5, tablet_peer->tablet()->metadata()->schema_version());
+    Schema schema = tablet_peer->tablet()->metadata()->schema();
+    ColumnSchema col_schema = schema.column(schema.find_column("string_val"));
+    ASSERT_FALSE(col_schema.has_read_default());
+    ASSERT_FALSE(col_schema.has_write_default());
+  }
+
+  // Clear a default value from a non-nullable column
+  {
+    gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("non_null_with_default")
+        ->RemoveDefault();
+    ASSERT_OK(table_alterer->Alter());
+    ASSERT_EQ(6, tablet_peer->tablet()->metadata()->schema_version());
+    Schema schema = tablet_peer->tablet()->metadata()->schema();
+    ColumnSchema col_schema = schema.column(schema.find_column("non_null_with_default"));
+    ASSERT_TRUE(col_schema.has_read_default());
+    ASSERT_FALSE(col_schema.has_write_default());
+  }
+
+  // Test that specifying a default of the wrong size fails.
+  // Note that, since the column's type isn't checked (or necessarily known)
+  // client-side, the server just receives some bytes, and, since the client
+  // stores 64-bit integer values as defaults for all integer-backed types, the
+  // value might be too large. We'll still be ok if we just truncate, though.
+  // This is actually how it works for regular add column and create table, but
+  // there the type of the column is known, so the truncation is backed up by
+  // column type-checking, and it can't be the case that there's too few bytes.
+  {
+    gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("non_null_with_default")
+        ->Default(KuduValue::CopyString("aaa"));
+    Status s = table_alterer->Alter();
+    ASSERT_TRUE(s.IsInvalidArgument());
+    ASSERT_STR_CONTAINS(s.ToString(), "wrong size for default value");
+    ASSERT_EQ(6, tablet_peer->tablet()->metadata()->schema_version());
+  }
+
+  // Test altering encoding, compression, and block size.
+  {
+    gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("string_val")
+        ->Encoding(KuduColumnStorageAttributes::DICT_ENCODING)
+        ->Compression(KuduColumnStorageAttributes::LZ4)
+        ->BlockSize(16 * 1024 * 1024);
+    ASSERT_OK(table_alterer->Alter());
+    ASSERT_EQ(7, tablet_peer->tablet()->metadata()->schema_version());
+    Schema schema = tablet_peer->tablet()->metadata()->schema();
+    ColumnSchema col_schema = schema.column(schema.find_column("string_val"));
+    ASSERT_EQ(KuduColumnStorageAttributes::DICT_ENCODING, col_schema.attributes().encoding);
+    ASSERT_EQ(KuduColumnStorageAttributes::LZ4, col_schema.attributes().compression);
+    ASSERT_EQ(16 * 1024 * 1024, col_schema.attributes().cfile_block_size);
+  }
+
   {
     const char *kRenamedTableName = "RenamedTable";
     gscoped_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
     ASSERT_OK(table_alterer
               ->RenameTo(kRenamedTableName)
               ->Alter());
-    ASSERT_EQ(3, tablet_peer->tablet()->metadata()->schema_version());
+    ASSERT_EQ(8, tablet_peer->tablet()->metadata()->schema_version());
     ASSERT_EQ(kRenamedTableName, tablet_peer->tablet()->metadata()->table_name());
 
     CatalogManager *catalog_manager = cluster_->mini_master()->master()->catalog_manager();

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/client/schema.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/schema.cc b/src/kudu/client/schema.cc
index 516dfcd..8009ac2 100644
--- a/src/kudu/client/schema.cc
+++ b/src/kudu/client/schema.cc
@@ -17,6 +17,7 @@
 
 #include "kudu/client/schema.h"
 
+#include <boost/optional.hpp>
 #include <glog/logging.h>
 #include <unordered_map>
 
@@ -204,8 +205,6 @@ Status KuduColumnSpec::ToColumnSchema(KuduColumnSchema* col) const {
   // Verify that the user isn't trying to use any methods that
   // don't make sense for CREATE.
   if (data_->has_rename_to) {
-    // TODO(KUDU-861): adjust these errors as this method will also be used for
-    // ALTER TABLE ADD COLUMN support.
     return Status::NotSupported("cannot rename a column during CreateTable",
                                 data_->name);
   }
@@ -254,6 +253,57 @@ Status KuduColumnSpec::ToColumnSchema(KuduColumnSchema* col) const {
   return Status::OK();
 }
 
+Status KuduColumnSpec::ToColumnSchemaDelta(ColumnSchemaDelta* col_delta) const {
+  if (data_->has_type) {
+    return Status::InvalidArgument("type provided for column schema delta", data_->name);
+  }
+  if (data_->has_nullable) {
+    return Status::InvalidArgument("nullability provided for column schema delta", data_->name);
+  }
+  if (data_->primary_key) {
+    return Status::InvalidArgument("primary key set for column schema delta", data_->name);
+  }
+
+  if (data_->has_rename_to) {
+    col_delta->new_name = boost::optional<string>(std::move(data_->rename_to));
+  }
+
+  if (data_->has_default) {
+    col_delta->default_value = boost::optional<Slice>(DefaultValueAsSlice());
+  }
+
+  if (data_->remove_default) {
+    col_delta->remove_default = true;
+  }
+
+  if (col_delta->remove_default && col_delta->default_value) {
+    return Status::InvalidArgument("new default set but default also removed", data_->name);
+  }
+
+  if (data_->has_encoding) {
+    col_delta->encoding =
+            boost::optional<EncodingType>(ToInternalEncodingType(data_->encoding));
+  }
+
+  if (data_->has_compression) {
+    col_delta->compression =
+            boost::optional<CompressionType>(ToInternalCompressionType(data_->compression));
+  }
+
+  if (data_->has_block_size) {
+    col_delta->cfile_block_size = boost::optional<int32_t>(data_->block_size);
+  }
+
+  return Status::OK();
+}
+
+Slice KuduColumnSpec::DefaultValueAsSlice() const {
+  if (!data_->has_default) {
+    return Slice();
+  }
+  return data_->default_val->data_->GetSlice();
+}
+
 
 ////////////////////////////////////////////////////////////
 // KuduSchemaBuilder

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/client/schema.h
----------------------------------------------------------------------
diff --git a/src/kudu/client/schema.h b/src/kudu/client/schema.h
index 5bdba4d..04eb553 100644
--- a/src/kudu/client/schema.h
+++ b/src/kudu/client/schema.h
@@ -27,6 +27,7 @@
 namespace kudu {
 
 class ColumnSchema;
+struct ColumnSchemaDelta;
 class KuduPartialRow;
 class Schema;
 class TestWorkload;
@@ -349,6 +350,10 @@ class KUDU_EXPORT KuduColumnSpec {
 
   Status ToColumnSchema(KuduColumnSchema* col) const;
 
+  Status ToColumnSchemaDelta(ColumnSchemaDelta* col_delta) const;
+
+  Slice DefaultValueAsSlice() const;
+
   // Owned.
   Data* data_;
 };

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/client/table_alterer-internal.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/table_alterer-internal.cc b/src/kudu/client/table_alterer-internal.cc
index 3ea2a16..ba86b55 100644
--- a/src/kudu/client/table_alterer-internal.cc
+++ b/src/kudu/client/table_alterer-internal.cc
@@ -86,29 +86,31 @@ Status KuduTableAlterer::Data::ToRequest(AlterTableRequestPB* req) {
         break;
       }
       case AlterTableRequestPB::ALTER_COLUMN:
-        // TODO(KUDU-861): support altering a column in the wire protocol.
-        // For now, we just give an error if the caller tries to do
-        // any operation other than rename.
+      {
+        // We only support rename column, change block_size, encoding, and compression
         if (s.spec->data_->has_type ||
-            s.spec->data_->has_encoding ||
-            s.spec->data_->has_compression ||
             s.spec->data_->has_nullable ||
-            s.spec->data_->primary_key ||
-            s.spec->data_->has_default ||
-            s.spec->data_->default_val ||
-            s.spec->data_->remove_default) {
-          return Status::NotSupported("cannot support AlterColumn of this type",
+            s.spec->data_->primary_key) {
+          return Status::NotSupported("unsupported alter operation",
                                       s.spec->data_->name);
         }
-        // We only support rename column
-        if (!s.spec->data_->has_rename_to) {
+        if (!s.spec->data_->has_rename_to &&
+            !s.spec->data_->has_default &&
+            !s.spec->data_->default_val &&
+            !s.spec->data_->remove_default &&
+            !s.spec->data_->has_encoding &&
+            !s.spec->data_->has_compression &&
+            !s.spec->data_->has_block_size) {
           return Status::InvalidArgument("no alter operation specified",
                                          s.spec->data_->name);
         }
-        pb_step->mutable_rename_column()->set_old_name(s.spec->data_->name);
-        pb_step->mutable_rename_column()->set_new_name(s.spec->data_->rename_to);
-        pb_step->set_type(AlterTableRequestPB::RENAME_COLUMN);
+        ColumnSchemaDelta col_delta(s.spec->data_->name);
+        RETURN_NOT_OK(s.spec->ToColumnSchemaDelta(&col_delta));
+        auto* alter_pb = pb_step->mutable_alter_column();
+        ColumnSchemaDeltaToPB(col_delta, alter_pb->mutable_delta());
+        pb_step->set_type(AlterTableRequestPB::ALTER_COLUMN);
         break;
+      }
       case AlterTableRequestPB::ADD_RANGE_PARTITION:
       {
         RowOperationsPBEncoder encoder(pb_step->mutable_add_range_partition()

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/client/value-internal.h
----------------------------------------------------------------------
diff --git a/src/kudu/client/value-internal.h b/src/kudu/client/value-internal.h
index c66a7bc..590a75c 100644
--- a/src/kudu/client/value-internal.h
+++ b/src/kudu/client/value-internal.h
@@ -55,6 +55,10 @@ class KuduValue::Data {
                                 DataType t,
                                 void** val_void);
 
+  // Return this KuduValue as a Slice. The KuduValue object retains ownership
+  // of the underlying data.
+  const Slice GetSlice();
+
  private:
   // Check that this value has the expected type 'type', returning
   // a nice error Status if not.

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/client/value.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/value.cc b/src/kudu/client/value.cc
index 7971748..2034b46 100644
--- a/src/kudu/client/value.cc
+++ b/src/kudu/client/value.cc
@@ -188,5 +188,23 @@ Status KuduValue::Data::CheckAndPointToString(const string& col_name,
   return Status::OK();
 }
 
+const Slice KuduValue::Data::GetSlice() {
+  switch (type_) {
+    case INT: {
+      return Slice(reinterpret_cast<uint8_t *>(&int_val_),
+                   sizeof(int64_t));
+    }
+    case FLOAT:
+      return Slice(reinterpret_cast<uint8_t*>(&float_val_),
+                   sizeof(float));
+    case DOUBLE:
+      return Slice(reinterpret_cast<uint8_t*>(&double_val_),
+                   sizeof(double));
+    case SLICE:
+      return slice_val_;
+  }
+  LOG(FATAL);
+}
+
 } // namespace client
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/common/common.proto
----------------------------------------------------------------------
diff --git a/src/kudu/common/common.proto b/src/kudu/common/common.proto
index c85e65b..c5eced0 100644
--- a/src/kudu/common/common.proto
+++ b/src/kudu/common/common.proto
@@ -101,6 +101,18 @@ message ColumnSchemaPB {
   optional int32 cfile_block_size = 10 [default=0];
 }
 
+message ColumnSchemaDeltaPB {
+  optional string name = 1;
+  optional string new_name = 2;
+
+  optional bytes default_value = 4;
+  optional bool remove_default = 5;
+
+  optional EncodingType encoding = 6;
+  optional CompressionType compression = 7;
+  optional int32 block_size = 8;
+}
+
 message SchemaPB {
   repeated ColumnSchemaPB columns = 1;
 }

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/common/schema.cc
----------------------------------------------------------------------
diff --git a/src/kudu/common/schema.cc b/src/kudu/common/schema.cc
index bd83088..2d44670 100644
--- a/src/kudu/common/schema.cc
+++ b/src/kudu/common/schema.cc
@@ -17,9 +17,10 @@
 
 #include "kudu/common/schema.h"
 
-#include <set>
 #include <algorithm>
+#include <set>
 
+#include "kudu/gutil/map-util.h"
 #include "kudu/gutil/stringprintf.h"
 #include "kudu/gutil/strings/join.h"
 #include "kudu/gutil/strings/strcat.h"
@@ -29,8 +30,7 @@
 
 namespace kudu {
 
-using std::set;
-using std::unordered_map;
+using std::shared_ptr;
 using std::unordered_set;
 
 // In a new schema, we typically would start assigning column IDs at 0. However, this
@@ -52,7 +52,44 @@ string ColumnStorageAttributes::ToString() const {
                              cfile_block_size);
 }
 
-// TODO: include attributes_.ToString() -- need to fix unit tests
+Status ColumnSchema::ApplyDelta(const ColumnSchemaDelta& col_delta) {
+  // This method does all validation up-front before making any changes to
+  // the schema, so that if we return an error then we are guaranteed to
+  // have had no effect
+  if (type_info()->physical_type() != BINARY) {
+    if (col_delta.default_value && col_delta.default_value->size() < type_info()->size()) {
+      return Status::InvalidArgument("wrong size for default value");
+    }
+  }
+
+  if (col_delta.new_name) {
+    name_ = *col_delta.new_name;
+  }
+
+  if (col_delta.default_value) {
+    const void* value = type_info()->physical_type() == BINARY ?
+                        reinterpret_cast<const void *>(&col_delta.default_value.get()) :
+                        reinterpret_cast<const void *>(col_delta.default_value->data());
+    write_default_ = std::make_shared<Variant>(type_info()->type(), value);
+  }
+
+  if (col_delta.remove_default) {
+    write_default_ = nullptr;
+  }
+
+  if (col_delta.encoding) {
+    attributes_.encoding = *col_delta.encoding;
+  }
+  if (col_delta.compression) {
+    attributes_.compression = *col_delta.compression;
+  }
+  if (col_delta.cfile_block_size) {
+    attributes_.cfile_block_size = *col_delta.cfile_block_size;
+  }
+  return Status::OK();
+}
+
+// TODO(wdb): include attributes_.ToString() -- need to fix unit tests
 // first
 string ColumnSchema::ToString() const {
   return strings::Substitute("$0[$1]",
@@ -417,8 +454,8 @@ Status SchemaBuilder::AddColumn(const string& name,
 }
 
 Status SchemaBuilder::RemoveColumn(const string& name) {
-  unordered_set<string>::const_iterator it_names;
-  if ((it_names = col_names_.find(name)) == col_names_.end()) {
+  unordered_set<string>::const_iterator it_names = col_names_.find(name);
+  if (it_names == col_names_.end()) {
     return Status::NotFound("The specified column does not exist", name);
   }
 
@@ -439,19 +476,18 @@ Status SchemaBuilder::RemoveColumn(const string& name) {
 }
 
 Status SchemaBuilder::RenameColumn(const string& old_name, const string& new_name) {
-  unordered_set<string>::const_iterator it_names;
-
   // check if 'new_name' is already in use
-  if ((it_names = col_names_.find(new_name)) != col_names_.end()) {
+  if (col_names_.find(new_name) != col_names_.end()) {
     return Status::AlreadyPresent("The column already exists", new_name);
   }
 
   // check if the 'old_name' column exists
-  if ((it_names = col_names_.find(old_name)) == col_names_.end()) {
+  unordered_set<string>::const_iterator it_names = col_names_.find(old_name);
+  if (it_names == col_names_.end()) {
     return Status::NotFound("The specified column does not exist", old_name);
   }
 
-  col_names_.erase(it_names);   // TODO: Should this one stay and marked as alias?
+  col_names_.erase(it_names);   // TODO(wdb): Should this one stay and marked as alias?
   col_names_.insert(new_name);
 
   for (ColumnSchema& col_schema : cols_) {
@@ -484,4 +520,31 @@ Status SchemaBuilder::AddColumn(const ColumnSchema& column, bool is_key) {
   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)) {
+    return Status::AlreadyPresent("The column already exists", *col_delta.new_name);
+  }
+
+  // check if the column exists
+  unordered_set<string>::const_iterator it_names = col_names_.find(col_delta.name);
+  if (it_names == col_names_.end()) {
+    return Status::NotFound("The specified column does not exist", col_delta.name);
+  }
+
+  for (ColumnSchema& col_schema : cols_) {
+    if (col_delta.name == col_schema.name()) {
+      RETURN_NOT_OK(col_schema.ApplyDelta(col_delta));
+      if (col_delta.new_name) {
+        // TODO(wdberkeley): Should the old one stay, marked as an alias?
+        col_names_.erase(it_names);
+        col_names_.insert(*col_delta.new_name);
+      }
+      return Status::OK();
+    }
+  }
+
+  LOG(FATAL) << "Should not reach here";
+}
+
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/common/schema.h
----------------------------------------------------------------------
diff --git a/src/kudu/common/schema.h b/src/kudu/common/schema.h
index b8656e9..bdf3ccf 100644
--- a/src/kudu/common/schema.h
+++ b/src/kudu/common/schema.h
@@ -17,6 +17,7 @@
 #ifndef KUDU_COMMON_SCHEMA_H
 #define KUDU_COMMON_SCHEMA_H
 
+#include <boost/optional.hpp>
 #include <functional>
 #include <glog/logging.h>
 #include <memory>
@@ -104,6 +105,37 @@ struct ColumnStorageAttributes {
   int32_t cfile_block_size;
 };
 
+// A struct representing changes to a ColumnSchema.
+//
+// In the future, as more complex alter operations need to be supported,
+// this may evolve into a class, but for now it's just POD.
+struct ColumnSchemaDelta {
+public:
+  explicit ColumnSchemaDelta(std::string name)
+      : name(std::move(name)),
+        remove_default(false) {
+  }
+
+  const std::string name;
+
+  boost::optional<std::string> new_name;
+
+  // NB: these properties of a column cannot be changed yet,
+  // ergo type and nullable should always be empty.
+  // TODO(wdberkeley) allow changing of type and nullability
+  boost::optional<DataType> type;
+
+  boost::optional<bool> nullable;
+
+  boost::optional<Slice> default_value;
+
+  bool remove_default;
+
+  boost::optional<EncodingType> encoding;
+  boost::optional<CompressionType> compression;
+  boost::optional<int32_t> cfile_block_size;
+};
+
 // The schema for a given column.
 //
 // Holds the data type as well as information about nullability & column name.
@@ -138,7 +170,6 @@ class ColumnSchema {
     if (write_default == read_default) {
       write_default_ = read_default_;
     } else if (write_default != NULL) {
-      DCHECK(read_default != NULL) << "Must have a read default";
       write_default_.reset(new Variant(type, write_default));
     }
   }
@@ -234,6 +265,11 @@ class ColumnSchema {
     return true;
   }
 
+  // Apply a ColumnSchemaDelta to this column schema, altering it according to
+  // the changes in the delta.
+  // The original column schema is not changed unless ApplyDelta returns OK.
+  Status ApplyDelta(const ColumnSchemaDelta& col_delta);
+
   // Returns extended attributes (such as encoding, compression, etc...)
   // associated with the column schema. The reason they are kept in a separate
   // struct is so that in the future, they may be moved out to a more
@@ -848,8 +884,11 @@ class SchemaBuilder {
                    const void *write_default);
 
   Status RemoveColumn(const string& name);
+
   Status RenameColumn(const string& old_name, const string& new_name);
 
+  Status ApplyColumnSchemaDelta(const ColumnSchemaDelta& col_delta);
+
  private:
   DISALLOW_COPY_AND_ASSIGN(SchemaBuilder);
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/common/wire_protocol.cc
----------------------------------------------------------------------
diff --git a/src/kudu/common/wire_protocol.cc b/src/kudu/common/wire_protocol.cc
index 0c3d23b..f8cf561 100644
--- a/src/kudu/common/wire_protocol.cc
+++ b/src/kudu/common/wire_protocol.cc
@@ -17,6 +17,7 @@
 
 #include "kudu/common/wire_protocol.h"
 
+#include <boost/optional.hpp>
 #include <string>
 #include <vector>
 
@@ -255,6 +256,53 @@ ColumnSchema ColumnSchemaFromPB(const ColumnSchemaPB& pb) {
                       attributes);
 }
 
+void ColumnSchemaDeltaToPB(const ColumnSchemaDelta& col_delta, ColumnSchemaDeltaPB *pb) {
+  pb->Clear();
+  pb->set_name(col_delta.name);
+  if (col_delta.new_name) {
+    pb->set_new_name(*col_delta.new_name);
+  }
+  if (col_delta.default_value) {
+    pb->set_default_value(col_delta.default_value->data(),
+                          col_delta.default_value->size());
+  }
+  if (col_delta.remove_default) {
+    pb->set_remove_default(true);
+  }
+  if (col_delta.encoding) {
+    pb->set_encoding(*col_delta.encoding);
+  }
+  if (col_delta.compression) {
+    pb->set_compression(*col_delta.compression);
+  }
+  if (col_delta.cfile_block_size) {
+    pb->set_block_size(*col_delta.cfile_block_size);
+  }
+}
+
+ColumnSchemaDelta ColumnSchemaDeltaFromPB(const ColumnSchemaDeltaPB& pb) {
+  ColumnSchemaDelta col_delta(pb.name());
+  if (pb.has_new_name()) {
+    col_delta.new_name = boost::optional<string>(pb.new_name());
+  }
+  if (pb.has_default_value()) {
+    col_delta.default_value = boost::optional<Slice>(Slice(pb.default_value()));
+  }
+  if (pb.has_remove_default()) {
+    col_delta.remove_default = true;
+  }
+  if (pb.has_encoding()) {
+    col_delta.encoding = boost::optional<EncodingType>(pb.encoding());
+  }
+  if (pb.has_compression()) {
+    col_delta.compression = boost::optional<CompressionType>(pb.compression());
+  }
+  if (pb.has_block_size()) {
+    col_delta.cfile_block_size = boost::optional<int32_t>(pb.block_size());
+  }
+  return col_delta;
+}
+
 Status ColumnPBsToSchema(const RepeatedPtrField<ColumnSchemaPB>& column_pbs,
                          Schema* schema) {
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/common/wire_protocol.h
----------------------------------------------------------------------
diff --git a/src/kudu/common/wire_protocol.h b/src/kudu/common/wire_protocol.h
index 6448282..d2743b7 100644
--- a/src/kudu/common/wire_protocol.h
+++ b/src/kudu/common/wire_protocol.h
@@ -32,6 +32,7 @@ namespace kudu {
 class Arena;
 class ColumnPredicate;
 class ColumnSchema;
+struct ColumnSchemaDelta;
 class ConstContiguousRow;
 class faststring;
 class HostPort;
@@ -85,6 +86,13 @@ void ColumnSchemaToPB(const ColumnSchema& schema, ColumnSchemaPB *pb, int flags
 // Return the ColumnSchema created from the specified protobuf.
 ColumnSchema ColumnSchemaFromPB(const ColumnSchemaPB& pb);
 
+// Convert the column schema delta `col_delta` to protobuf.
+void ColumnSchemaDeltaToPB(const ColumnSchemaDelta& col_delta, ColumnSchemaDeltaPB *pb);
+
+// Return the ColumnSchemaDelta created from the protobuf `pb`.
+// The protobuf must outlive the returned ColumnSchemaDelta.
+ColumnSchemaDelta ColumnSchemaDeltaFromPB(const ColumnSchemaDeltaPB& pb);
+
 // Convert the given list of ColumnSchemaPB objects into a Schema object.
 //
 // Returns InvalidArgument if the provided columns don't make a valid Schema

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/integration-tests/alter_table-randomized-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/alter_table-randomized-test.cc b/src/kudu/integration-tests/alter_table-randomized-test.cc
index 55b5318..ab7cd5a 100644
--- a/src/kudu/integration-tests/alter_table-randomized-test.cc
+++ b/src/kudu/integration-tests/alter_table-randomized-test.cc
@@ -36,10 +36,9 @@
 namespace kudu {
 
 using client::KuduClient;
-using client::KuduClientBuilder;
 using client::KuduColumnSchema;
+using client::KuduColumnStorageAttributes;
 using client::KuduError;
-using client::KuduInsert;
 using client::KuduScanner;
 using client::KuduSchema;
 using client::KuduSchemaBuilder;
@@ -61,6 +60,18 @@ using strings::SubstituteAndAppend;
 const char* kTableName = "test-table";
 const int kMaxColumns = 30;
 const uint32_t kMaxRangePartitions = 32;
+const vector<KuduColumnStorageAttributes::CompressionType> kCompressionTypes =
+    { KuduColumnStorageAttributes::NO_COMPRESSION,
+      KuduColumnStorageAttributes::SNAPPY,
+      KuduColumnStorageAttributes::LZ4,
+      KuduColumnStorageAttributes::ZLIB };
+const vector <KuduColumnStorageAttributes::EncodingType> kInt32Encodings =
+      { KuduColumnStorageAttributes::PLAIN_ENCODING,
+        KuduColumnStorageAttributes::RLE,
+        KuduColumnStorageAttributes::BIT_SHUFFLE };
+// A block size of 0 applies the server-side default.
+const vector<int32_t> kBlockSizes = {0, 2 * 1024 * 1024,
+                                     4 * 1024 * 1024, 8 * 1024 * 1024};
 
 class AlterTableRandomized : public KuduTest {
  public:
@@ -115,6 +126,10 @@ struct RowState {
   // We ensure that we never insert or update to this value except in the case of
   // NULLable columns.
   static const int32_t kNullValue = 0xdeadbeef;
+  // We use this special value to denote default values.
+  // We ensure that we never insert or update to this value except when the
+  // column should be assigned its current default value.
+  static const int32_t kDefaultValue = 0xbabecafe;
   vector<pair<string, int32_t>> cols;
 
   string ToString() const {
@@ -141,6 +156,7 @@ struct TableState {
       : rand_(SeedRandom()) {
     col_names_.push_back("key");
     col_nullable_.push_back(false);
+    col_defaults_.push_back(0);
     AddRangePartition();
   }
 
@@ -185,7 +201,7 @@ struct TableState {
     int32_t key = GetRandomNewRowKey();
 
     int32_t seed = rand_.Next();
-    if (seed == RowState::kNullValue) {
+    if (seed == RowState::kNullValue || seed == RowState::kDefaultValue) {
       seed++;
     }
 
@@ -195,6 +211,8 @@ struct TableState {
       int32_t val;
       if (col_nullable_[i] && seed % 2 == 1) {
         val = RowState::kNullValue;
+      } else if (seed % 3 == 0) {
+        val = RowState::kDefaultValue;
       } else {
         val = seed;
       }
@@ -209,6 +227,11 @@ struct TableState {
 
     auto r = new RowState;
     r->cols = data;
+    for (int i = 1; i < r->cols.size(); i++) {
+      if (r->cols[i].second == RowState::kDefaultValue) {
+        r->cols[i].second = col_defaults_[i];
+      }
+    }
     rows_[key].reset(r);
     return true;
   }
@@ -230,6 +253,7 @@ struct TableState {
   void AddColumnWithDefault(const string& name, int32_t def, bool nullable) {
     col_names_.push_back(name);
     col_nullable_.push_back(nullable);
+    col_defaults_.push_back(def);
     for (auto& e : rows_) {
       e.second->cols.push_back(make_pair(name, def));
     }
@@ -240,15 +264,30 @@ struct TableState {
     int index = col_it - col_names_.begin();
     col_names_.erase(col_it);
     col_nullable_.erase(col_nullable_.begin() + index);
+    col_defaults_.erase(col_defaults_.begin() + index);
     for (auto& e : rows_) {
       e.second->cols.erase(e.second->cols.begin() + index);
     }
   }
 
-  void RenameColumn(const string& existing_name, string new_name) {
+  void RenameColumn(const string& existing_name, const string& new_name) {
     auto iter = std::find(col_names_.begin(), col_names_.end(), existing_name);
     CHECK(iter != col_names_.end());
-    *iter = std::move(new_name);
+    *iter = new_name;
+
+    for (auto& e : rows_) {
+      for (auto& pair : e.second->cols) {
+        if (pair.first == existing_name) {
+          pair.first = new_name;
+        }
+      }
+    }
+  }
+
+  void ChangeDefault(const string& name, int32_t new_def) {
+    auto col_it = std::find(col_names_.begin(), col_names_.end(), name);
+    CHECK(col_it != col_names_.end());
+    col_defaults_[col_it - col_names_.begin()] = new_def;
   }
 
   pair<int32_t, int32_t> AddRangePartition() {
@@ -294,6 +333,10 @@ struct TableState {
   // Has the same length as col_names_.
   vector<bool> col_nullable_;
 
+  // For each column, its current write default.
+  // Has the same length as col_names_.
+  vector<int32_t> col_defaults_;
+
   map<int32_t, unique_ptr<RowState>> rows_;
 
   // The lower and upper bounds of all range partitions in the table.
@@ -359,6 +402,7 @@ struct MirrorTable {
     for (int i = 1; i < num_columns(); i++) {
       int32_t val = rand * i;
       if (val == RowState::kNullValue) val++;
+      if (val == RowState::kDefaultValue) val++;
       if (ts_.col_nullable_[i] && val % 2 == 1) {
         val = RowState::kNullValue;
       }
@@ -391,13 +435,19 @@ struct MirrorTable {
 
     int step_count = 1 + ts_.rand_.Uniform(10);
     for (int step = 0; step < step_count; step++) {
-      int r = ts_.rand_.Uniform(4);
+      int r = ts_.rand_.Uniform(7);
       if (r < 1 && num_columns() < kMaxColumns) {
         AddAColumn(table_alterer.get());
       } else if (r < 2 && num_columns() > 1) {
         DropAColumn(table_alterer.get());
+      } else if (r < 3 && num_columns() > 1) {
+        RenameAColumn(table_alterer.get());
+      } else if (r < 4 && num_columns() > 1) {
+        ChangeADefault(table_alterer.get());
+      } else if (r < 5 && num_columns() > 1) {
+        ChangeAStorageAttribute(table_alterer.get());
       } else if (num_range_partitions() == 0 ||
-                 (r < 3 && num_range_partitions() < kMaxRangePartitions)) {
+                 (r < 6 && num_range_partitions() < kMaxRangePartitions)) {
         AddARangePartition(schema, table_alterer.get());
       } else {
         DropARangePartition(schema, table_alterer.get());
@@ -441,10 +491,42 @@ struct MirrorTable {
     string new_name = ts_.GetRandomNewColumnName();
     LOG(INFO) << "Renaming column " << original_name << " to " << new_name;
     table_alterer->AlterColumn(original_name)->RenameTo(new_name);
-    ts_.RenameColumn(original_name, std::move(new_name));
+    ts_.RenameColumn(original_name, new_name);
+  }
+
+  void ChangeADefault(KuduTableAlterer* table_alterer) {
+    string name = ts_.GetRandomExistingColumnName();
+    int32_t new_def = ts_.rand_.Next();
+    if (new_def == RowState::kNullValue || new_def == RowState::kDefaultValue) {
+      new_def++;
+    }
+    LOG(INFO) << "Changing default of column " << name << " to " << new_def;
+    table_alterer->AlterColumn(name)->Default(KuduValue::FromInt(new_def));
+    ts_.ChangeDefault(name, new_def);
+  }
+
+  void ChangeAStorageAttribute(KuduTableAlterer* table_alterer) {
+    string name = ts_.GetRandomExistingColumnName();
+    int type = ts_.rand_.Uniform(3);
+    if (type == 0) {
+      int i = ts_.rand_.Uniform(kCompressionTypes.size());
+      LOG(INFO) << "Changing compression of column " << name <<
+          " to " << kCompressionTypes[i];
+      table_alterer->AlterColumn(name)->Compression(kCompressionTypes[i]);
+    } else if (type == 1) {
+      int i = ts_.rand_.Uniform(kInt32Encodings.size());
+      LOG(INFO) << "Changing encoding of column " << name <<
+          " to " << kInt32Encodings[i];
+      table_alterer->AlterColumn(name)->Encoding(kInt32Encodings[i]);
+    } else {
+      int i = ts_.rand_.Uniform(kBlockSizes.size());
+      LOG(INFO) << "Changing block size of column " << name <<
+          " to " << kBlockSizes[i];
+      table_alterer->AlterColumn(name)->BlockSize(kBlockSizes[i]);
+    }
   }
 
-  void AddARangePartition(KuduSchema& schema, KuduTableAlterer* table_alterer) {
+  void AddARangePartition(const KuduSchema& schema, KuduTableAlterer* table_alterer) {
     auto bounds = ts_.AddRangePartition();
     LOG(INFO) << "Adding range partition: [" << bounds.first << ", " << bounds.second << ")"
               << " resulting partitions: ("
@@ -545,9 +627,16 @@ struct MirrorTable {
       case UPDATE: op.reset(table->NewUpdate()); break;
       case DELETE: op.reset(table->NewDelete()); break;
     }
-    for (const auto& d : data) {
+    for (int i = 0; i < data.size(); i++) {
+      const auto& d = data[i];
       if (d.second == RowState::kNullValue) {
         CHECK_OK(op->mutable_row()->SetNull(d.first));
+      } else if (d.second == RowState::kDefaultValue) {
+        if (ts_.col_defaults_[i] == RowState::kNullValue) {
+          CHECK_OK(op->mutable_row()->SetNull(d.first));
+        } else {
+          CHECK_OK(op->mutable_row()->SetInt32(d.first, ts_.col_defaults_[i]));
+        }
       } else {
         CHECK_OK(op->mutable_row()->SetInt32(d.first, d.second));
       }

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/integration-tests/alter_table-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/alter_table-test.cc b/src/kudu/integration-tests/alter_table-test.cc
index 6ae9be2..5e8757a 100644
--- a/src/kudu/integration-tests/alter_table-test.cc
+++ b/src/kudu/integration-tests/alter_table-test.cc
@@ -345,6 +345,116 @@ TEST_F(AlterTableTest, TestAddNullableColumnWithoutDefault) {
   EXPECT_EQ("(int32 c0=16777216, int32 c1=1, int32 new=NULL)", rows[1]);
 }
 
+// Test altering a column to add a default value, change a default value, and
+// remove a default value.
+TEST_F(AlterTableTest, TestAddChangeRemoveColumnDefaultValue) {
+  {
+    unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AddColumn("newInt32")->Type(KuduColumnSchema::INT32);
+    table_alterer->AddColumn("newString")->Type(KuduColumnSchema::STRING);
+    ASSERT_OK(table_alterer->Alter());
+  }
+
+  InsertRows(0, 1);
+  ASSERT_OK(tablet_peer_->tablet()->Flush());
+
+  {
+    unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("newInt32")->Default(KuduValue::FromInt(12345));
+    table_alterer->AlterColumn("newString")->Default(KuduValue::CopyString("taco"));
+    ASSERT_OK(table_alterer->Alter());
+  }
+
+  InsertRows(1, 1);
+  ASSERT_OK(tablet_peer_->tablet()->Flush());
+
+  vector<string> rows;
+  ScanToStrings(&rows);
+  ASSERT_EQ(2, rows.size());
+  EXPECT_EQ("(int32 c0=0, int32 c1=0, int32 newInt32=NULL, string newString=NULL)", rows[0]);
+  EXPECT_EQ("(int32 c0=16777216, int32 c1=1, int32 newInt32=12345, string newString=\"taco\")",
+            rows[1]);
+
+  {
+    unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("newInt32")->Default(KuduValue::FromInt(54321));
+    table_alterer->AlterColumn("newString")->Default(KuduValue::CopyString("ocat"));
+    ASSERT_OK(table_alterer->Alter());
+  }
+
+  InsertRows(2, 1);
+  ASSERT_OK(tablet_peer_->tablet()->Flush());
+
+  ScanToStrings(&rows);
+  ASSERT_EQ(3, rows.size());
+  EXPECT_EQ("(int32 c0=0, int32 c1=0, int32 newInt32=NULL, string newString=NULL)", rows[0]);
+  EXPECT_EQ("(int32 c0=16777216, int32 c1=1, int32 newInt32=12345, string newString=\"taco\")",
+            rows[1]);
+  EXPECT_EQ("(int32 c0=33554432, int32 c1=2, int32 newInt32=54321, string newString=\"ocat\")",
+            rows[2]);
+
+  {
+    unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("newInt32")->RemoveDefault();
+    table_alterer->AlterColumn("newString")->RemoveDefault();
+    // Add an extra rename step
+    table_alterer->AlterColumn("newString")->RenameTo("newNewString");
+    ASSERT_OK(table_alterer->Alter());
+  }
+
+  InsertRows(3, 1);
+  ASSERT_OK(tablet_peer_->tablet()->Flush());
+
+  ScanToStrings(&rows);
+  ASSERT_EQ(4, rows.size());
+  EXPECT_EQ("(int32 c0=0, int32 c1=0, int32 newInt32=NULL, string newNewString=NULL)", rows[0]);
+  EXPECT_EQ("(int32 c0=16777216, int32 c1=1, int32 newInt32=12345, string newNewString=\"taco\")",
+            rows[1]);
+  EXPECT_EQ("(int32 c0=33554432, int32 c1=2, int32 newInt32=54321, string newNewString=\"ocat\")",
+            rows[2]);
+  EXPECT_EQ("(int32 c0=50331648, int32 c1=3, int32 newInt32=NULL, string newNewString=NULL)",
+            rows[3]);
+
+  {
+    unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->DropColumn("newInt32");
+    table_alterer->DropColumn("newNewString");
+    ASSERT_OK(table_alterer->Alter());
+  }
+}
+
+// Test for a bug seen when an alter table creates an empty RLE block and the
+// block is subsequently scanned.
+TEST_F(AlterTableTest, TestAlterEmptyRLEBlock) {
+  // Add an RLE-encoded column
+  {
+    unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AddColumn("rle")->Type(KuduColumnSchema::INT32)
+      ->Encoding(client::KuduColumnStorageAttributes::RLE);
+    ASSERT_OK(table_alterer->Alter());
+  }
+
+  // Insert some rows
+  InsertRows(0, 3);
+
+  // Now alter the column
+  {
+    unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AlterColumn("rle")->RenameTo("new_rle");
+    ASSERT_OK(table_alterer->Alter());
+  }
+
+  // Now scan the table, which would trigger a CHECK failure
+  // on trying to seek to position 0 in an empty RLE block
+  {
+    vector<string> rows;
+    ScanToStrings(&rows);
+    EXPECT_EQ("(int32 c0=0, int32 c1=0, int32 new_rle=NULL)", rows[0]);
+    EXPECT_EQ("(int32 c0=16777216, int32 c1=1, int32 new_rle=NULL)", rows[1]);
+    EXPECT_EQ("(int32 c0=33554432, int32 c1=2, int32 new_rle=NULL)", rows[2]);
+  }
+}
+
 // Verify that, if a tablet server is down when an alter command is issued,
 // it will eventually receive the command when it restarts.
 TEST_F(AlterTableTest, TestAlterOnTSRestart) {

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index d42dc25..ede90d6 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -1348,19 +1348,19 @@ Status CatalogManager::ApplyAlterSchemaSteps(const SysTablesEntryPB& current_pb,
         }
 
         if (cur_schema.is_key_column(step.drop_column().name())) {
-          return Status::InvalidArgument("cannot remove a key column");
+          return Status::InvalidArgument("cannot remove a key column",
+                                         step.drop_column().name());
         }
 
         RETURN_NOT_OK(builder.RemoveColumn(step.drop_column().name()));
         break;
       }
-
+      // Remains for backwards compatibility
       case AlterTableRequestPB::RENAME_COLUMN: {
         if (!step.has_rename_column()) {
           return Status::InvalidArgument("RENAME_COLUMN missing column info");
         }
-
-        // TODO: In theory we can rename a key
+        // TODO(wdb): In theory we can rename a key (KUDU-1626).
         if (cur_schema.is_key_column(step.rename_column().old_name())) {
           return Status::InvalidArgument("cannot rename a key column");
         }
@@ -1370,9 +1370,18 @@ Status CatalogManager::ApplyAlterSchemaSteps(const SysTablesEntryPB& current_pb,
                         step.rename_column().new_name()));
         break;
       }
-
-      // TODO: EDIT_COLUMN
-
+      case AlterTableRequestPB::ALTER_COLUMN: {
+        if (!step.has_alter_column()) {
+          return Status::InvalidArgument("ALTER_COLUMN missing column info");
+        }
+        const ColumnSchemaDelta col_delta = ColumnSchemaDeltaFromPB(step.alter_column().delta());
+        // TODO(wdb): In theory we can rename a key (KUDU-1626).
+        if (cur_schema.is_key_column(col_delta.name)) {
+          return Status::InvalidArgument("cannot alter a key column", col_delta.name);
+        }
+        RETURN_NOT_OK(builder.ApplyColumnSchemaDelta(col_delta));
+        break;
+      }
       default: {
         return Status::InvalidArgument("Invalid alter schema step type", step.ShortDebugString());
       }
@@ -1565,7 +1574,8 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB* req,
     switch (step.type()) {
       case AlterTableRequestPB::ADD_COLUMN:
       case AlterTableRequestPB::DROP_COLUMN:
-      case AlterTableRequestPB::RENAME_COLUMN: {
+      case AlterTableRequestPB::RENAME_COLUMN: // Fallthrough intended.
+      case AlterTableRequestPB::ALTER_COLUMN: {
         alter_schema_steps.emplace_back(step);
         break;
       }
@@ -1574,7 +1584,6 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB* req,
         alter_partitioning_steps.emplace_back(step);
         break;
       }
-      case AlterTableRequestPB::ALTER_COLUMN:
       case AlterTableRequestPB::UNKNOWN: {
         return Status::InvalidArgument("Invalid alter step type", step.ShortDebugString());
       }

http://git-wip-us.apache.org/repos/asf/kudu/blob/e16a541a/src/kudu/master/master.proto
----------------------------------------------------------------------
diff --git a/src/kudu/master/master.proto b/src/kudu/master/master.proto
index fb02dc1..9dfd0a2 100644
--- a/src/kudu/master/master.proto
+++ b/src/kudu/master/master.proto
@@ -425,10 +425,8 @@ message AlterTableRequestPB {
     UNKNOWN = 0;
     ADD_COLUMN = 1;
     DROP_COLUMN = 2;
+    // Deprecated. Replaced by AlterColumn.
     RENAME_COLUMN = 3;
-
-    // TODO(KUDU-861): this will subsume RENAME_COLUMN, but not yet implemented
-    // on the master side.
     ALTER_COLUMN = 4;
     ADD_RANGE_PARTITION = 5;
     DROP_RANGE_PARTITION = 6;
@@ -443,11 +441,15 @@ message AlterTableRequestPB {
     // Name of the column to drop.
     required string name = 1;
   }
+  // Deprecated. Replaced by AlterColumn.
   message RenameColumn {
     // Name of the column to rename;
     required string old_name = 1;
     required string new_name = 2;
   }
+  message AlterColumn {
+    required ColumnSchemaDeltaPB delta = 1;
+  }
   message AddRangePartition {
     // A set of row operations containing the lower and upper range bound for
     // the range partition to add or drop.
@@ -468,6 +470,7 @@ message AlterTableRequestPB {
     optional RenameColumn rename_column = 4;
     optional AddRangePartition add_range_partition = 5;
     optional DropRangePartition drop_range_partition = 6;
+    optional AlterColumn alter_column = 7;
   }
 
   required TableIdentifierPB table = 1;


Mime
View raw message