You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/11/10 06:01:24 UTC

[kudu-CR] KUDU-180. Fix handling of defaults when creating tables 

Hello Dan Burkert, Jean-Daniel Cryans,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: KUDU-180. Fix handling of defaults when creating tables
......................................................................

KUDU-180. Fix handling of defaults when creating tables

This fixes an issue that was experienced when creating tables from the
Java client when those tables had a column with a default value.
Previously, the client would send the default value as the
'read_default' protobuf field, and the server would just write that
default into the table schema. This would result in the write_default
being left unset, and thus insertions which did not specify the column
would insert NULL rather than inserting the column's default.

This didn't affect tables created from the C++ API, because the C++
client set both the read and write default fields when creating a table.

Because the separation of "read" and "write" defaults is an internal
concept, we shouldn't really have exposed it in the client-facing
protobuf in the first place. Unfortunately, that ship sailed long ago
and the same protobuf is used internally as is used in the client-facing
wire protocol, and changing it now would be a huge amount of work to do
without breaking compatibility.

Thus, the fix here is to document that the server will only look at the
"read_default" field sent by the client when adding columns. The server
then propagates this field as the initial write_default for any
newly-added columns.

A new test is added in Java which reproduced the issue but now passes.

Change-Id: Ia76c7cb599292fac4d7a8edf163d59a1574e897b
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M src/kudu/client/client.cc
M src/kudu/client/table_alterer-internal.cc
M src/kudu/common/common.proto
M src/kudu/common/wire_protocol.cc
M src/kudu/common/wire_protocol.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
8 files changed, 195 insertions(+), 39 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia76c7cb599292fac4d7a8edf163d59a1574e897b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>

[kudu-CR] KUDU-180. Fix handling of defaults when creating tables

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-180. Fix handling of defaults when creating tables
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5031/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:

Line 185:         // We expect two of the inserts to fail when we try to set NULL values for
> Wanna add a check that we get two?
that's already implicit because there are only three expected rows despite trying to insert five


http://gerrit.cloudera.org:8080/#/c/5031/1/src/kudu/common/common.proto
File src/kudu/common/common.proto:

Line 89:   // ignore 'write_default_value'.
> Could you also add a note about where clients should look for the default v
Done


Line 89:   // ignore 'write_default_value'.
> Being pedantic a bit, but the server won't 'ignore' write_default_value; it
Done


http://gerrit.cloudera.org:8080/#/c/5031/1/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:

Line 297:     ColumnSchemaToPB(col, col_pb);
> Shouldn't 'flags' propagate into this call?
good catch


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia76c7cb599292fac4d7a8edf163d59a1574e897b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-180. Fix handling of defaults when creating tables

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: KUDU-180. Fix handling of defaults when creating tables
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5031/1/src/kudu/common/common.proto
File src/kudu/common/common.proto:

Line 89:   // ignore 'write_default_value'.
Could you also add a note about where clients should look for the default value for existing columns, e.g. how to interpret the fields when calling GetTableSchema


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia76c7cb599292fac4d7a8edf163d59a1574e897b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-180. Fix handling of defaults when creating tables

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.

Change subject: KUDU-180. Fix handling of defaults when creating tables
......................................................................


KUDU-180. Fix handling of defaults when creating tables

This fixes an issue that was experienced when creating tables from the
Java client when those tables had a column with a default value.
Previously, the client would send the default value as the
'read_default' protobuf field, and the server would just write that
default into the table schema. This would result in the write_default
being left unset, and thus insertions which did not specify the column
would insert NULL rather than inserting the column's default.

This didn't affect tables created from the C++ API, because the C++
client set both the read and write default fields when creating a table.

Because the separation of "read" and "write" defaults is an internal
concept, we shouldn't really have exposed it in the client-facing
protobuf in the first place. Unfortunately, that ship sailed long ago
and the same protobuf is used internally as is used in the client-facing
wire protocol, and changing it now would be a huge amount of work to do
without breaking compatibility.

Thus, the fix here is to document that the server will only look at the
"read_default" field sent by the client when adding columns. The server
then propagates this field as the initial write_default for any
newly-added columns.

A new test is added in Java which reproduced the issue but now passes.

Change-Id: Ia76c7cb599292fac4d7a8edf163d59a1574e897b
Reviewed-on: http://gerrit.cloudera.org:8080/5031
Reviewed-by: Jean-Daniel Cryans <jd...@apache.org>
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M src/kudu/client/client.cc
M src/kudu/client/table_alterer-internal.cc
M src/kudu/common/common.proto
M src/kudu/common/wire_protocol.cc
M src/kudu/common/wire_protocol.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
8 files changed, 199 insertions(+), 40 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, but someone else must approve
  Jean-Daniel Cryans: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia76c7cb599292fac4d7a8edf163d59a1574e897b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-180. Fix handling of defaults when creating tables

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-180. Fix handling of defaults when creating tables
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5031/1/src/kudu/common/common.proto
File src/kudu/common/common.proto:

Line 89:   // ignore 'write_default_value'.
> Could you also add a note about where clients should look for the default v
Being pedantic a bit, but the server won't 'ignore' write_default_value; it'll fail a request if it was set but doesn't match read_default_value.


http://gerrit.cloudera.org:8080/#/c/5031/1/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:

Line 297:     ColumnSchemaToPB(col, col_pb);
Shouldn't 'flags' propagate into this call?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia76c7cb599292fac4d7a8edf163d59a1574e897b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-180. Fix handling of defaults when creating tables

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: KUDU-180. Fix handling of defaults when creating tables
......................................................................


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia76c7cb599292fac4d7a8edf163d59a1574e897b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-180. Fix handling of defaults when creating tables

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-180. Fix handling of defaults when creating tables
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5031/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:

Line 185:         // We expect two of the inserts to fail when we try to set NULL values for
Wanna add a check that we get two?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia76c7cb599292fac4d7a8edf163d59a1574e897b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-180. Fix handling of defaults when creating tables

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-180. Fix handling of defaults when creating tables
......................................................................


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia76c7cb599292fac4d7a8edf163d59a1574e897b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-180. Fix handling of defaults when creating tables

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-180. Fix handling of defaults when creating tables
......................................................................

KUDU-180. Fix handling of defaults when creating tables

This fixes an issue that was experienced when creating tables from the
Java client when those tables had a column with a default value.
Previously, the client would send the default value as the
'read_default' protobuf field, and the server would just write that
default into the table schema. This would result in the write_default
being left unset, and thus insertions which did not specify the column
would insert NULL rather than inserting the column's default.

This didn't affect tables created from the C++ API, because the C++
client set both the read and write default fields when creating a table.

Because the separation of "read" and "write" defaults is an internal
concept, we shouldn't really have exposed it in the client-facing
protobuf in the first place. Unfortunately, that ship sailed long ago
and the same protobuf is used internally as is used in the client-facing
wire protocol, and changing it now would be a huge amount of work to do
without breaking compatibility.

Thus, the fix here is to document that the server will only look at the
"read_default" field sent by the client when adding columns. The server
then propagates this field as the initial write_default for any
newly-added columns.

A new test is added in Java which reproduced the issue but now passes.

Change-Id: Ia76c7cb599292fac4d7a8edf163d59a1574e897b
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M src/kudu/client/client.cc
M src/kudu/client/table_alterer-internal.cc
M src/kudu/common/common.proto
M src/kudu/common/wire_protocol.cc
M src/kudu/common/wire_protocol.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
8 files changed, 199 insertions(+), 40 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia76c7cb599292fac4d7a8edf163d59a1574e897b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>