[kudu-CR] [WIP] KUDU-861 Support changing default, storage attributes

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [WIP] KUDU-861 Support changing default, storage attributes
..


Patch Set 5:

Build Started http://104.196.14.100/job/kudu-gerrit/3415/

-- 
To view, visit http://gerrit.cloudera.org:8080/4310
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] [WIP] KUDU-861 Support changing default, storage attributes

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4310

to look at the new patch set (#5).

Change subject: [WIP] KUDU-861 Support changing default, storage attributes
..

[WIP] KUDU-861 Support changing default, storage attributes

This patch adds support for adding, changing, or removing column defaults.
It's straightforward, but a bit throny because the column's type (in Java,
even the column name) is not known when the alter is configured. So, for
defaults, the server must best-effort deserialize the value as the correct
type. I think the solution to this in the future might be to make AlterColumn
a method on a KuduTable, rather than a KuduClient, so some type information
is available.

There's another bit of potentially weird or unexpected behavior. Consider:
1. Client writes data to table. Data is flushed to CFiles.
2. Client adds a column with a read default.
3. Some compactions happen. RowSets that are compacted have the new
default written into them.
4. Client changes the read default.
5. As-yet-uncompacted RowSets will now read and compact with the new
default value.
There's no real way to control what gets what default.
This should at least be highlighted in the docs.

Changing type and nullability of a column is still unsupported (doing so
is much more complicated than this).

Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
M src/kudu/client/client-test.cc
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
14 files changed, 717 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/4310/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4310
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] [WIP] KUDU-861 Support changing default, storage attributes

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [WIP] KUDU-861 Support changing default, storage attributes
..


Patch Set 3:

No problem putting a patch up for it. It was assigned to me for more than a 
year with no action ;-) Feel free to re-assign to yourself

-- 
To view, visit http://gerrit.cloudera.org:8080/4310
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] [WIP] KUDU-861 Support changing default, storage attributes

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: [WIP] KUDU-861 Support changing default, storage attributes
..


Patch Set 3:

> (1 comment)
 > 
 > didn't get to review the patch yet, just a quick comment on
 > back-compat

No problem-- that's not hard to do. I appreciate you glancing at it but you can 
dodge reviewing this for now. I mostly wanted to see if Jenkins would like the 
C++ part. Also, I realized I was slightly rude by submitting a quasi-patch for 
this when you're still assigned to the JIRA and I didn't ask :( sorry

-- 
To view, visit http://gerrit.cloudera.org:8080/4310
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] [WIP] KUDU-861 Support changing default, storage attributes

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: [WIP] KUDU-861 Support changing default, storage attributes
..


Patch Set 3:

> Build Started http://104.196.14.100/job/kudu-gerrit/3229/

Fixed compilation whoopsie that doesn't show up on OS X. Should fail b/c Java 
client not changed yet.

-- 
To view, visit http://gerrit.cloudera.org:8080/4310
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] [WIP] KUDU-861 Support changing default, storage attributes

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [WIP] KUDU-861 Support changing default, storage attributes
..


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/3229/

-- 
To view, visit http://gerrit.cloudera.org:8080/4310
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [WIP] KUDU-861 Support changing default, storage attributes

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4310

to look at the new patch set (#3).

Change subject: [WIP] KUDU-861 Support changing default, storage attributes
..

[WIP] KUDU-861 Support changing default, storage attributes

This patch adds support for adding, changing, or removing column defaults.
It adds support for these operations to the wire protocol, master, and
C++ client, and folds column renaming into the same PB message as alter
column more generally. It is not backwards-compatible.

Some thorny things remain. The column's type isn't checked (or necessarily
known) client-side, the server just receives some bytes for the default
value, and, since the client stores 64-bit integer values as defaults for
all integer-backed types, the value might be too large. We'll be ok, we
just truncate. 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.

Possible solutions:
1. Leave it as it is. It works and shouldn't break, but users may have a
weird experience like setting an integer column to have default value "",
getting no error, and ending up with a default of 24929 = 0x61616161.
2. Put a discriminated union type structure in the PB message so some type
checking can be done, i.e. INT vs FLOAT not INT32 vs. INT16.
3. Change KuduValue to require more type information + 2 above.

Changing type and nullability of a column is still unsupported (doing so
is much more complicated than this).

Java client support is upcoming.

Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
---
M src/kudu/client/client-test.cc
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
11 files changed, 476 insertions(+), 47 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/4310/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4310
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [WIP] KUDU-861 Support changing default, storage attributes

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4310

to look at the new patch set (#2).

Change subject: [WIP] KUDU-861 Support changing default, storage attributes
..

[WIP] KUDU-861 Support changing default, storage attributes

This patch adds support for adding, changing, or removing column defaults.
It adds support for these operations to the wire protocol, master, and
C++ client, and folds column renaming into the same PB message as alter
column more generally. It is not backwards-compatible.

Some thorny things remain. The column's type isn't checked (or necessarily
known) client-side, the server just receives some bytes for the default
value, and, since the client stores 64-bit integer values as defaults for
all integer-backed types, the value might be too large. We'll be ok, we
just truncate. 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.

Possible solutions:
1. Leave it as it is. It works and shouldn't break, but users may have a
weird experience like setting an integer column to have default value "",
getting no error, and ending up with a default of 24929 = 0x61616161.
2. Put a discriminated union type structure in the PB message so some type
checking can be done, i.e. INT vs FLOAT not INT32 vs. INT16.
3. Change KuduValue to require more type information + 2 above.

Changing type and nullability of a column is still unsupported (doing so
is much more complicated than this).

Java client support is upcoming.

Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
---
M src/kudu/client/client-test.cc
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
11 files changed, 476 insertions(+), 47 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/4310/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4310
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [WIP] KUDU-861 Support changing default, storage attributes

[Will Berkeley (Code Review)]

Will Berkeley has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4310

Change subject: [WIP] KUDU-861 Support changing default, storage attributes
..

[WIP] KUDU-861 Support changing default, storage attributes

This patch adds support for adding, changing, or removing column defaults.
It adds support for these operations to the wire protocol, master, and
C++ client, and folds column renaming into the same PB message as alter
column more generally. It is not backwards-compatible.

Some thorny things remain. The column's type isn't checked (or necessarily
known) client-side, the server just receives some bytes for the default
value, and, since the client stores 64-bit integer values as defaults for
all integer-backed types, the value might be too large. We'll be ok, we
just truncate. 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.

Possible solutions:
1. Leave it as it is. It works and shouldn't break, but users may have a
weird experience like setting an integer column to have default value "",
getting no error, and ending up with a default of 24929 = 0x61616161.
2. Put a discriminated union type structure in the PB message so some type
checking can be done, i.e. INT vs FLOAT not INT32 vs. INT16.
3. Change KuduValue to require more type information + 2 above.

Changing type and nullability of a column is still unsupported (doing so
is much more complicated than this).

Java client support is upcoming.

Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
---
M src/kudu/client/client-test.cc
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
11 files changed, 476 insertions(+), 47 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/4310/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4310
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 

[kudu-CR] [WIP] KUDU-861 Support changing default, storage attributes

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [WIP] KUDU-861 Support changing default, storage attributes
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/3227/

-- 
To view, visit http://gerrit.cloudera.org:8080/4310
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No