You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2019/02/26 22:53:16 UTC
[kudu-CR] schema: Fix ColumnSchema::Equals() usage
Hello Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/12605
to review the following change.
Change subject: schema: Fix ColumnSchema::Equals() usage
......................................................................
schema: Fix ColumnSchema::Equals() usage
In commit 4be39feef4907550c16f9d2b34430d098b9f1b21 the signature of
ColumnSchema::Equals() was changed from taking a boolean flag for
checking whether the defaults matched to taking an enum for describing a
broader set of comparison options. However, a few call sites were not
updated when that change was made, and due to type coercion several call
sites ended up with weaker comparison semantics.
This patch reinstates the behavior prior to the merge of the
above-mentioned patch for the problematic call sites and updates the
function signature of ColumnSchema::Equals() to treat the comparison
enum as strongly typed to avoid similar problems in the future.
Updated tests to add coverage for the relevant call sites.
Change-Id: I416e4a44d0ed259d251d52a663b72acd79b46f33
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.h
8 files changed, 84 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/12605/1
--
To view, visit http://gerrit.cloudera.org:8080/12605
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I416e4a44d0ed259d251d52a663b72acd79b46f33
Gerrit-Change-Number: 12605
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] schema: Fix ColumnSchema::Equals() usage
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12605
to look at the new patch set (#2).
Change subject: schema: Fix ColumnSchema::Equals() usage
......................................................................
schema: Fix ColumnSchema::Equals() usage
In commit 4be39feef4907550c16f9d2b34430d098b9f1b21 the signature of
ColumnSchema::Equals() was changed from taking a boolean flag for
checking whether the defaults matched to taking an enum for describing a
broader set of comparison options. However, a few call sites were not
updated when that change was made, and due to type coercion several call
sites ended up with weaker comparison semantics.
This patch reinstates the behavior prior to the merge of the
above-mentioned patch for the problematic call sites and updates the
function signature of ColumnSchema::Equals() to treat the comparison
enum as strongly typed to avoid similar problems in the future.
Updated tests to add coverage for the relevant call sites.
Change-Id: I416e4a44d0ed259d251d52a663b72acd79b46f33
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.h
8 files changed, 87 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/12605/2
--
To view, visit http://gerrit.cloudera.org:8080/12605
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I416e4a44d0ed259d251d52a663b72acd79b46f33
Gerrit-Change-Number: 12605
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] schema: Fix ColumnSchema::Equals() usage
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12605 )
Change subject: schema: Fix ColumnSchema::Equals() usage
......................................................................
schema: Fix ColumnSchema::Equals() usage
In commit 4be39feef4907550c16f9d2b34430d098b9f1b21 the signature of
ColumnSchema::Equals() was changed from taking a boolean flag for
checking whether the defaults matched to taking an enum for describing a
broader set of comparison options. However, a few call sites were not
updated when that change was made, and due to type coercion several call
sites ended up with weaker comparison semantics.
This patch reinstates the behavior prior to the merge of the
above-mentioned patch for the problematic call sites and updates the
function signature of ColumnSchema::Equals() to treat the comparison
enum as strongly typed to avoid similar problems in the future.
Updated tests to add coverage for the relevant call sites.
Change-Id: I416e4a44d0ed259d251d52a663b72acd79b46f33
Reviewed-on: http://gerrit.cloudera.org:8080/12605
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.h
8 files changed, 87 insertions(+), 11 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/12605
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I416e4a44d0ed259d251d52a663b72acd79b46f33
Gerrit-Change-Number: 12605
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] schema: Fix ColumnSchema::Equals() usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12605 )
Change subject: schema: Fix ColumnSchema::Equals() usage
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12605/1/src/kudu/common/column_predicate-test.cc
File src/kudu/common/column_predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/12605/1/src/kudu/common/column_predicate-test.cc@1470
PS1, Line 1470: using TestColumnPredicateDeathTest = TestColumnPredicate;
> Test suites with a name ending in "DeathTest" are run before all other tests.
TIL. Thanks.
--
To view, visit http://gerrit.cloudera.org:8080/12605
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I416e4a44d0ed259d251d52a663b72acd79b46f33
Gerrit-Change-Number: 12605
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Tue, 26 Feb 2019 23:56:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: Fix ColumnSchema::Equals() usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12605 )
Change subject: schema: Fix ColumnSchema::Equals() usage
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12605
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I416e4a44d0ed259d251d52a663b72acd79b46f33
Gerrit-Change-Number: 12605
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 28 Feb 2019 00:42:13 +0000
Gerrit-HasComments: No
[kudu-CR] schema: Fix ColumnSchema::Equals() usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12605 )
Change subject: schema: Fix ColumnSchema::Equals() usage
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12605/1/src/kudu/common/column_predicate-test.cc
File src/kudu/common/column_predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/12605/1/src/kudu/common/column_predicate-test.cc@1470
PS1, Line 1470: using TestColumnPredicateDeathTest = TestColumnPredicate;
What's the purpose of this aliasing?
--
To view, visit http://gerrit.cloudera.org:8080/12605
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I416e4a44d0ed259d251d52a663b72acd79b46f33
Gerrit-Change-Number: 12605
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 26 Feb 2019 23:15:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: Fix ColumnSchema::Equals() usage
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12605 )
Change subject: schema: Fix ColumnSchema::Equals() usage
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12605/1/src/kudu/common/column_predicate-test.cc
File src/kudu/common/column_predicate-test.cc:
http://gerrit.cloudera.org:8080/#/c/12605/1/src/kudu/common/column_predicate-test.cc@1470
PS1, Line 1470: using TestColumnPredicateDeathTest = TestColumnPredicate;
> What's the purpose of this aliasing?
That works as well as defining a new class with DeathTest in the name, which is recommended by the gtest documentation at https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#death-test-naming so that the framework can place additional thread safety related safeguards in place.
--
To view, visit http://gerrit.cloudera.org:8080/12605
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I416e4a44d0ed259d251d52a663b72acd79b46f33
Gerrit-Change-Number: 12605
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Tue, 26 Feb 2019 23:53:51 +0000
Gerrit-HasComments: Yes