You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2018/07/18 00:35:50 UTC
[kudu-CR] memrowset: support iteration with is deleted virtual column
Hello Mike Percy, Grant Henke, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/10968
to review the following change.
Change subject: memrowset: support iteration with is_deleted virtual column
......................................................................
memrowset: support iteration with is_deleted virtual column
This patch rounds out the MemRowSet changes for incremental backups.
Taken together, it is now possible to iterate on a specific time range and
to learn which rows were deleted during that time range.
The patch introduces a very basic concept of a "virtual column", defined as
a statically defined column with a name, type, and defaults. A Kudu
subsystem that wishes to interact with a virtual column needs to first
figure out if the projection includes it (via ColumnSchema::Equals()). When
projected, the virtual column's data will be entirely default; it's the
subsystem's responsibility to fill in something meaningful afterwards.
There's absolutely no robustness here. For example, there's currently
nothing stopping someone from creating a real column with the same name. It
remains to be seen how usable this bare-bones abstraction will be; so far
it's sufficient for the MemRowSet.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
5 files changed, 106 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/1
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 15: Verified+1
Overriding Jenkins, unrelated Spark test failure.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 15
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 06 Aug 2018 22:15:24 +0000
Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 12: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 12
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 02 Aug 2018 20:21:14 +0000
Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Alexey Serbin, Dan Burkert, Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10968
to look at the new patch set (#13).
Change subject: schema: add is_deleted virtual column
......................................................................
schema: add is_deleted virtual column
This patch introduces a very basic concept of a "virtual column". Virtual
columns borrow from other databases in that they are columns that, rather
than being backed by physical data, are instead backed by Kudu itself. They
may not be part of a schema during table creation/alteration, but may be
added to projections during a scan.
Kudu's virtual columns are defined as logical data types. As data types are
not user-defined, there's no danger of a "collision" between a virtual
column and a physical column as there would be if a virtual column occupied
a well-defined name.
A Kudu subsystem on the scan path that wishes to interact with a virtual
column needs to first figure out if the projection includes it. When
projected, the virtual column's data will be either some default or null
(depending on exactly how it was defined in the projection); it's the
subsystem's responsibility to fill in something meaningful afterwards.
Beyond the basic definition, this patch introduces an IS_DELETED virtual
column derived from BOOL. IS_DELETED will be used to support incremental
backups by describing whether a row was deleted between two timestamps.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/common.proto
M src/kudu/common/schema.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
6 files changed, 119 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/13
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 13
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.h@412
PS3, Line 412: Boolean
> nit: Boolean column (reads like a typo to me, otherwise)
Done
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.cc@142
PS3, Line 142: $$IS_DELETED$$
> This seems to imply that column names of the form $$FOO$$ are reserved for
Not really. Todd pointed me at the Postgres documentation (https://www.postgresql.org/docs/current/static/ddl-system-columns.html) but they don't use any kind of separators for system column names.
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.cc@144
PS3, Line 144: false /* is_nullable */
> protip: if you instead use the syntax /* is_nullable= */ false, clang-tidy
Done
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Jul 2018 21:37:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10968
to look at the new patch set (#6).
Change subject: schema: add is_deleted virtual column
......................................................................
schema: add is_deleted virtual column
This patch introduces a very basic concept of a "virtual column", defined as
a static column with a name, type, and defaults. A Kudu subsystem that
wishes to interact with a virtual column needs to first figure out if the
projection includes it (via ColumnSchema::Equals()). When projected, the
virtual column's data will be entirely default; it's the subsystem's
responsibility to fill in something meaningful afterwards.
This first cut is hardly robust. It remains to be seen how usable this
bare-bones abstraction will be; so far it's sufficient for the MemRowSet.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
4 files changed, 47 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/6
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@673
PS11, Line 673: case IS_DELETED:
> In my opinion this isn't an appropriate use for FALLTHRUGH_INTENDED. It sh
The downside of adding FALLTHROUGH_INTENDED is that it's just obfuscating noise; no one's taken by surprise that statement-less cases fall through.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 12
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 02 Aug 2018 18:52:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Alexey Serbin, Dan Burkert, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10968
to look at the new patch set (#15).
Change subject: schema: add is_deleted virtual column
......................................................................
schema: add is_deleted virtual column
This patch introduces a very basic concept of a "virtual column". Virtual
columns borrow from other databases in that they are columns that, rather
than being backed by physical data, are instead backed by Kudu itself. They
may not be part of a schema during table creation/alteration, but may be
added to projections during a scan.
Kudu's virtual columns are defined as logical data types. As data types are
not user-defined, there's no danger of a "collision" between a virtual
column and a physical column as there would be if a virtual column occupied
a well-defined name.
A Kudu subsystem on the scan path that wishes to interact with a virtual
column needs to first figure out if the projection includes it. When
projected, the virtual column's data will be either some default or null
(depending on exactly how it was defined in the projection); it's the
subsystem's responsibility to fill in something meaningful afterwards.
Beyond the basic definition, this patch introduces an IS_DELETED virtual
column derived from BOOL. IS_DELETED will be used to support incremental
backups by describing whether a row was deleted between two timestamps.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/common.proto
M src/kudu/common/schema.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
6 files changed, 120 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/15
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 15
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 15: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 15
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 06 Aug 2018 20:31:01 +0000
Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.cc@142
PS3, Line 142: $$IS_DELETED$$
> Not really. Todd pointed me at the Postgres documentation (https://www.post
It looks like Oracle calls them pseudocolumns: https://docs.oracle.com/cd/B19306_01/server.102/b14200/pseudocolumns.htm
Postgres says if you quote the name in SQL you can access a conflicting column name.
I guess this special syntax of $$...$$ is better than that, assuming it doesn't get mangled by Impala or Spark.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Jul 2018 22:26:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Dan Burkert, Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10968
to look at the new patch set (#10).
Change subject: schema: add is_deleted virtual column
......................................................................
schema: add is_deleted virtual column
This patch introduces a very basic concept of a "virtual column". Virtual
columns borrow from other databases in that they are columns that, rather
than being backed by physical data, are instead backed by Kudu itself. They
may not be part of a schema during table creation/alteration, but may be
added to projections during a scan.
Kudu's virtual columns are defined as logical data types. As data types are
not user-defined, there's no danger of a "collision" between a virtual
column and a physical column as there would be if a virtual column occupied
a well-defined name.
A Kudu subsystem on the scan path that wishes to interact with a virtual
column needs to first figure out if the projection includes it. When
projected, the virtual column's data will be either some default or null
(depending on exactly how it was defined in the projection); it's the
subsystem's responsibility to fill in something meaningful afterwards.
Beyond the basic definition, this patch introduces an IS_DELETED virtual
column derived from BOOL. IS_DELETED will be used to support incremental
backups by describing whether a row was deleted between two timestamps.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/common.proto
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
7 files changed, 122 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/10
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 10
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@158
PS11, Line 158: static bool IsVirtual() {
: return false;
: }
> Is that possible? I thought the DataTypeTraits template was intentionally e
Woops, I see. That would be possible if not those exact type-coded references like TypeTraitsClass::min_value() and alike. But it seems that was designed to be like this.
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@673
PS11, Line 673: case IS_DELETED:
> As another point of evidence, Clang doesn't warn for statement-less cases,
That makes sense. I thought the code style just mandates having those FALLTHROUGH_INTENDED.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 11
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 02 Aug 2018 20:08:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@673
PS11, Line 673: case IS_DELETED:
> The downside of adding FALLTHROUGH_INTENDED is that it's just obfuscating n
As another point of evidence, Clang doesn't warn for statement-less cases, and clang::fallthrough is why this is a macro and not just a comment convention: https://clang.llvm.org/docs/AttributeReference.html#fallthrough-clang-fallthrough
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 12
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 02 Aug 2018 18:55:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10968/9/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/10968/9/src/kudu/common/schema.cc@143
PS9, Line 143: $$
how about "$$KUDU."
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 9
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 25 Jul 2018 20:34:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 13: Code-Review+1
LGTM
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 13
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 02 Aug 2018 23:24:52 +0000
Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 7:
(1 comment)
> I'm curious how this will plumb through all the way to the client. Have you started working on any such patches for TabletService, etc?
Not yet, no.
http://gerrit.cloudera.org:8080/#/c/10968/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/10968/7/src/kudu/master/catalog_manager.cc@1302
PS7, Line 1302: if (ContainsKey(Schema::kVirtualColumnNames, col_name)) {
> Instead of the set, maybe we should just reserve all identifiers of the for
Yeah, Grant had the same feedback. I'm open to making that change; just wanted to make sure this is headed in the right direction first (see my back and forth with Dan).
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Jul 2018 22:57:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 10:
(2 comments)
Nice, looks like not too much boilerplate!
http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/schema.h@881
PS10, Line 881: static const char* kReservedColNamePrefix;
No longer needed?
http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h@625
PS10, Line 625: return DataTypeTraits<BOOL>::name();
Should this be "is_deleted"? The derived types above don't delegate to their physical type.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 10
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 02 Aug 2018 03:14:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
schema: add is_deleted virtual column
This patch introduces a very basic concept of a "virtual column". Virtual
columns borrow from other databases in that they are columns that, rather
than being backed by physical data, are instead backed by Kudu itself. They
may not be part of a schema during table creation/alteration, but may be
added to projections during a scan.
Kudu's virtual columns are defined as logical data types. As data types are
not user-defined, there's no danger of a "collision" between a virtual
column and a physical column as there would be if a virtual column occupied
a well-defined name.
A Kudu subsystem on the scan path that wishes to interact with a virtual
column needs to first figure out if the projection includes it. When
projected, the virtual column's data will be either some default or null
(depending on exactly how it was defined in the projection); it's the
subsystem's responsibility to fill in something meaningful afterwards.
Beyond the basic definition, this patch introduces an IS_DELETED virtual
column derived from BOOL. IS_DELETED will be used to support incremental
backups by describing whether a row was deleted between two timestamps.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Reviewed-on: http://gerrit.cloudera.org:8080/10968
Reviewed-by: Grant Henke <gr...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/common/common.proto
M src/kudu/common/schema.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
6 files changed, 120 insertions(+), 27 deletions(-)
Approvals:
Grant Henke: Looks good to me, but someone else must approve
Alexey Serbin: Looks good to me, but someone else must approve
Adar Dembo: Verified
Mike Percy: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 16
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 11:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@158
PS11, Line 158: static bool IsVirtual() {
: return false;
: }
nit: maybe, move this into the DataTypeTraits template itself and have custom specialization just for IS_DELETED?
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@673
PS11, Line 673: case IS_DELETED:
nit: add FALLTHROUGH_INTENDED ?
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@763
PS11, Line 763: case IS_DELETED:
nit: add FALLTHROUGH_INTENDED ?
http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h@625
PS10, Line 625: return DataTypeTraits<BOOL>::name();
> It was at first, but that led to a row dump that looked like this:
But if it's name is 'bool', then how to distinguish between a column of virtual type and regular bool in a row dump? Or we don't need to differentiate between those in row dumps?
From the other side, what if the new type was named 'VIRTULAL_BOOL' or 'VBOOL' instead of 'IS_DELETED'?
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 11
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 02 Aug 2018 14:30:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 6:
> > - last-updated-timestamp per-row. If I understand tablet internals correctly, this can't ever be provided with per-row granularity, since we dispose of per-row timestamps during compaction after the ancient history watermark. I think the best we could do here is to provide it per scan batch.
>
> I'm not sure this is correct, but I'm following up with Mike (and looking at the code) to find out for sure.
Mike says that you're right; UNDO GC can and will discard the INSERT UNDO representing the insertion of a row, even if the row is still alive. So once the row's insertion is behind the ancient history mark, there's no guarantee of there being even a single UNDO or REDO record for the row, which means we can't provide a last-updated-timestamp (or created-timestamp, or whatever).
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Jul 2018 18:56:25 +0000
Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Alexey Serbin, Dan Burkert, Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10968
to look at the new patch set (#12).
Change subject: schema: add is_deleted virtual column
......................................................................
schema: add is_deleted virtual column
This patch introduces a very basic concept of a "virtual column". Virtual
columns borrow from other databases in that they are columns that, rather
than being backed by physical data, are instead backed by Kudu itself. They
may not be part of a schema during table creation/alteration, but may be
added to projections during a scan.
Kudu's virtual columns are defined as logical data types. As data types are
not user-defined, there's no danger of a "collision" between a virtual
column and a physical column as there would be if a virtual column occupied
a well-defined name.
A Kudu subsystem on the scan path that wishes to interact with a virtual
column needs to first figure out if the projection includes it. When
projected, the virtual column's data will be either some default or null
(depending on exactly how it was defined in the projection); it's the
subsystem's responsibility to fill in something meaningful afterwards.
Beyond the basic definition, this patch introduces an IS_DELETED virtual
column derived from BOOL. IS_DELETED will be used to support incremental
backups by describing whether a row was deleted between two timestamps.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/common.proto
M src/kudu/common/schema.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
6 files changed, 145 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/12
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 12
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 12: Code-Review+1
Overall lgtm
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 12
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 02 Aug 2018 19:04:01 +0000
Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 10:
(2 comments)
> Nice, looks like not too much boilerplate!
Check out the MRS patch that follows; there's a bit of a gotcha in that there's no mandated nullability or read-default.
http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/schema.h@881
PS10, Line 881: static const char* kReservedColNamePrefix;
> No longer needed?
Done
http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h@625
PS10, Line 625: return DataTypeTraits<BOOL>::name();
> Should this be "is_deleted"? The derived types above don't delegate to the
It was at first, but that led to a row dump that looked like this:
(string key="row", uint32 val=5, is_deleted is_deleted=true)
I thought that was confusing and ugly (granted in part because I named the column 'is_deleted'), so I added this bit of delegation.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 10
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 02 Aug 2018 04:02:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 14:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10968/14/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/10968/14/src/kudu/master/catalog_manager.cc@1324
PS14, Line 1324: Status s = TypeEncodingInfo::Get(ti, col.attributes().encoding, &dummy);
You don't need to fix this in this patch since you didn't make it, but I don't like this validation style. Maybe we should add an isValid method instead of trying to get an instance of TypeEncodingInfo we don't use.
http://gerrit.cloudera.org:8080/#/c/10968/14/src/kudu/master/catalog_manager.cc@1330
PS14, Line 1330: if (ti->is_virtual()) {
Nit: Move this check above the encoding check since this is a more significant error and fixing encoding issues would just lead to this.
http://gerrit.cloudera.org:8080/#/c/10968/14/src/kudu/master/catalog_manager.cc@1332
PS14, Line 1332: "may not create virtual column of type '$0'", ti->name()));
Maybe include the column name too?
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 14
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 06 Aug 2018 17:33:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.cc@144
PS3, Line 144: false /* is_nullable= *
> Unfortunately, the clang-tidy syntax requires the comment to come before th
Oops. Okay, I've reformatted this.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Jul 2018 22:36:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 9:
(1 comment)
This implementation is so straightfoward and the risks limited enough in scope that I'm frankly pretty happy with this approach for the time being. Here's why:
> breaking change
I think we can target this precisely enough (perhaps make it $$kudu.is_deleted$$ that the probability of breaking someone in real life is vanishingly small.
> looking at 'is-deleted' in isolation, I think it would be better to have an optional bitset that gets plumbed through which holds the deleted status. [...] It's clear that in the long run incremental (CDC) scans should use a sparse row representation
I agree that a sparse row layout would be more optimal for CDC, but implementing a virtual column now doesn't preclude us from doing that in the future, and implementing an optimal solution isn't a priority for us right now.
> last-updated-timestamp per-row. If I understand tablet internals correctly, this can't ever be provided with per-row granularity, since we dispose of per-row timestamps during compaction after the ancient history watermark.
While it isn't possible to get that information today, if we wanted this feature in the future we could implement it by keeping created / modified information on a per-row basis in a separate cfile or perhaps in one of the indexes. It wouldn't necessarily be accurate for existing tables, but going forward from the time of upgrade the modified information could theoretically be maintained.
http://gerrit.cloudera.org:8080/#/c/10968/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/10968/7/src/kudu/master/catalog_manager.cc@1302
PS7, Line 1302: if (Schema::is_column_name_reserved(col_name)) {
> Yeah, Grant had the same feedback. I'm open to making that change; just wan
I don't think it's unreasonable to reserve $$kudu.*$$ or $$kudu_*$$ because this is so easy to implement and meets all of the other requirements for the is_deleted feature except for namespacing.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 9
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 25 Jul 2018 20:33:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@673
PS11, Line 673: case IS_DELETED:
> As another point of evidence, Clang doesn't warn for statement-less cases,
This is an interesting perspective Dan, something stylistic I have thought about in the past without a satisfying conclusion and I think your argument makes a lot of sense.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 12
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 02 Aug 2018 18:58:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10968/6/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/10968/6/src/kudu/common/schema.cc@140
PS6, Line 140: static const bool kFalse = false;
> Can you comment why the read default should be false? It's a little differe
Sure, will add a comment here.
http://gerrit.cloudera.org:8080/#/c/10968/6/src/kudu/common/schema.cc@145
PS6, Line 145: &kFalse,
> Nit: Comment with /* read_default=*/ to match others.
Done
http://gerrit.cloudera.org:8080/#/c/10968/6/src/kudu/common/schema.cc@154
PS6, Line 154: // TODO(adar): C++11 provides a single-arg constructor
> Nit: Do we assign individual names to given TODO's? I was under the impress
We follow clang-tidy's recommendation, which is derived from the Google style guide:
https://clang.llvm.org/extra/clang-tidy/checks/google-readability-todo.html
It allows for both bug numbers (preferred) as well as usernames.
http://gerrit.cloudera.org:8080/#/c/10968/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/10968/6/src/kudu/master/catalog_manager.cc@1302
PS6, Line 1302: if (ContainsKey(Schema::kVirtualColumnNames, col_name)) {
> Would it be more future proof to prevent the $$ pattern in general?
Perhaps, but I'd like to collect more feedback on this approach before I make that change.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Jul 2018 04:50:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10968/6/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/10968/6/src/kudu/common/schema.cc@140
PS6, Line 140: static const bool kFalse = false;
Can you comment why the read default should be false? It's a little different than the usual case where a non-nullable column was added and historical values are being read. In this case, there are no values to read ever.
http://gerrit.cloudera.org:8080/#/c/10968/6/src/kudu/common/schema.cc@145
PS6, Line 145: &kFalse,
Nit: Comment with /* read_default=*/ to match others.
http://gerrit.cloudera.org:8080/#/c/10968/6/src/kudu/common/schema.cc@154
PS6, Line 154: // TODO(adar): C++11 provides a single-arg constructor
Nit: Do we assign individual names to given TODO's? I was under the impression we used Jiras or nothing.
http://gerrit.cloudera.org:8080/#/c/10968/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/10968/6/src/kudu/master/catalog_manager.cc@1302
PS6, Line 1302: if (ContainsKey(Schema::kVirtualColumnNames, col_name)) {
Would it be more future proof to prevent the $$ pattern in general?
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Jul 2018 19:50:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 15: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 15
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 07 Aug 2018 00:10:45 +0000
Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 14:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10968/14/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/10968/14/src/kudu/master/catalog_manager.cc@1324
PS14, Line 1324: Status s = TypeEncodingInfo::Get(ti, col.attributes().encoding, &dummy);
> You don't need to fix this in this patch since you didn't make it, but I do
Fair point, but of the four usages of Get(), this is the only one that ignores the returned argument. So I think the style abuse is acceptable for the time being.
http://gerrit.cloudera.org:8080/#/c/10968/14/src/kudu/master/catalog_manager.cc@1330
PS14, Line 1330: if (ti->is_virtual()) {
> Nit: Move this check above the encoding check since this is a more signific
Done
http://gerrit.cloudera.org:8080/#/c/10968/14/src/kudu/master/catalog_manager.cc@1332
PS14, Line 1332: "may not create virtual column of type '$0'", ti->name()));
> Maybe include the column name too?
Done
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 14
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 06 Aug 2018 19:29:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 6:
> Plus the current implementation has the advantage of being dead simple to implement, which is important because we've tabled several design decisions until a working backup and restore MVP is ready, so the faster we can get there, the better.
I'm all for a simple solution, but in this case it's a solution which introduces a breaking changes the public API of Kudu, and from what I've gathered the long-term solution won't require such a breaking change.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Jul 2018 20:28:02 +0000
Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10968
to look at the new patch set (#5).
Change subject: schema: add is_deleted virtual column
......................................................................
schema: add is_deleted virtual column
This patch introduces a very basic concept of a "virtual column", defined as
a static column with a name, type, and defaults. A Kudu subsystem that
wishes to interact with a virtual column needs to first figure out if the
projection includes it (via ColumnSchema::Equals()). When projected, the
virtual column's data will be entirely default; it's the subsystem's
responsibility to fill in something meaningful afterwards.
This first cut is hardly robust. It remains to be seen how usable this
bare-bones abstraction will be; so far it's sufficient for the MemRowSet.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
4 files changed, 47 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/5
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10968
to look at the new patch set (#3).
Change subject: schema: add is_deleted virtual column
......................................................................
schema: add is_deleted virtual column
This patch introduces a very basic concept of a "virtual column", defined as
a static column with a name, type, and defaults. A Kudu subsystem that
wishes to interact with a virtual column needs to first figure out if the
projection includes it (via ColumnSchema::Equals()). When projected, the
virtual column's data will be entirely default; it's the subsystem's
responsibility to fill in something meaningful afterwards.
This first cut is hardly robust. It remains to be seen how usable this
bare-bones abstraction will be; so far it's sufficient for the MemRowSet.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
4 files changed, 49 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/3
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 14
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 6:
Todd was the main advocate for the virtual column approach; I'd be interested in his take too. I've responded to a few things below:
> First, if looking at 'is-deleted' in isolation, I think it would be better to have an optional bitset that gets plumbed through which holds the deleted status. As I understand it a boolean flag indicating whether to retain deleted rows is already going to have to get plumbed through, so this wouldn't be a big complication. As a bonus, bitsets are more efficient in terms of memory overhead (1 bit per row vs 1 byte per row). Ultimately I think this will have a cleaner implementation than checking against known column values. It's clear that in the long run incremental (CDC) scans should use a sparse row representation, in which case re-using the existing row-batch layout is out the window, anyway.
The plumbing is quite different though:
1. The boolean flag you allude to is part of the RowIteratorOptions passed to each iterator at construction time, and it's just a single boolean for the entire iterator. This plumbing has already been done in commit 0fbebd67c.
2. The bitset you're proposing would need to be per-RowBlock in rowwise iterators and per-ColumnBlock in columnwise iterators, which means plumbing through NextBlock, MaterializeColumn, and several other iterator methods.
Plus the current implementation has the advantage of being dead simple to implement, which is important because we've tabled several design decisions until a working backup and restore MVP is ready, so the faster we can get there, the better.
> - last-updated-timestamp per-row. If I understand tablet internals correctly, this can't ever be provided with per-row granularity, since we dispose of per-row timestamps during compaction after the ancient history watermark. I think the best we could do here is to provide it per scan batch.
I'm not sure this is correct, but I'm following up with Mike (and looking at the code) to find out for sure.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Jul 2018 18:29:34 +0000
Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Dan Burkert, Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10968
to look at the new patch set (#9).
Change subject: schema: add is_deleted virtual column
......................................................................
schema: add is_deleted virtual column
This patch introduces a very basic concept of a "virtual column", defined as
a static column with a name, type, and defaults. A Kudu subsystem that
wishes to interact with a virtual column needs to first figure out if the
projection includes it (via ColumnSchema::Equals()). When projected, the
virtual column's data will be entirely default; it's the subsystem's
responsibility to fill in something meaningful afterwards.
This first cut is hardly robust. It remains to be seen how usable this
bare-bones abstraction will be; so far it's sufficient for the MemRowSet.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
4 files changed, 104 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/9
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 9
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.h@412
PS3, Line 412: Boolean
nit: Boolean column (reads like a typo to me, otherwise)
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.cc@142
PS3, Line 142: $$IS_DELETED$$
This seems to imply that column names of the form $$FOO$$ are reserved for internal Kudu use. Did you do any research into common schemes for indicating reserved column names?
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.cc@144
PS3, Line 144: false /* is_nullable */
protip: if you instead use the syntax /* is_nullable= */ false, clang-tidy will validate the variable name against the declaration for you and warn if they don't match.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Jul 2018 18:57:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Dan Burkert, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10968
to look at the new patch set (#8).
Change subject: schema: add is_deleted virtual column
......................................................................
schema: add is_deleted virtual column
This patch introduces a very basic concept of a "virtual column", defined as
a static column with a name, type, and defaults. A Kudu subsystem that
wishes to interact with a virtual column needs to first figure out if the
projection includes it (via ColumnSchema::Equals()). When projected, the
virtual column's data will be entirely default; it's the subsystem's
responsibility to fill in something meaningful afterwards.
This first cut is hardly robust. It remains to be seen how usable this
bare-bones abstraction will be; so far it's sufficient for the MemRowSet.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
4 files changed, 103 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/8
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 15
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:
http://gerrit.cloudera.org:8080/#/c/10968/3/src/kudu/common/schema.cc@144
PS3, Line 144: false /* is_nullable= *
> Done
Unfortunately, the clang-tidy syntax requires the comment to come before the value. It doesn't work if you put it after. See https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Jul 2018 22:22:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 6:
I had a long discussion about this offline with Adar. As I understand it, 'virtual columns' are a generalized feature meant to allow querying the 'is-deleted' status of a row without having to plumb through special handling in many layers of the stack (client, RPC, tserver). Eventually this feature could be extended to more per-row metadata. I'm sympathetic to this motivation, but nonetheless I don't come down in favor of this feature.
First, if looking at 'is-deleted' in isolation, I think it would be better to have an optional bitset that gets plumbed through which holds the deleted status. As I understand it a boolean flag indicating whether to retain deleted rows is already going to have to get plumbed through, so this wouldn't be a big complication. As a bonus, bitsets are more efficient in terms of memory overhead (1 bit per row vs 1 byte per row). Ultimately I think this will have a cleaner implementation than checking against known column values. It's clear that in the long run incremental (CDC) scans should use a sparse row representation, in which case re-using the existing row-batch layout is out the window, anyway.
Second, it doesn't seem to me that a generalized virtual column feature would be useful for other metadata. Here's a partial list of attributes that have been thrown around, and an explanation of why I don't think virtual columns are appropriate:
- per-row tablet server and per-row tablet information. It would be preferable to expose this on the scan batch, it's not necessary to hold a copy of this per row, and it's not necessary to send it across the wire.
- last-updated-timestamp per-row. If I understand tablet internals correctly, this can't ever be provided with per-row granularity, since we dispose of per-row timestamps during compaction after the ancient history watermark. I think the best we could do here is to provide it per scan batch.
- DRS ID / CFile ID / block (page) ID per row. I think this use-case is a real stretch, motivation wise. I think there will never be much demand for this info, and what little demand there is would be better served by just providing aggregate counts of # of DRS's, # of CFiles, and # of blocks scanned per row batch.
To wrap up this already-too-long comment, I'd favor not introducing this abstraction unless we can identify a realistic second use-case for it.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Jul 2018 16:41:14 +0000
Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Dan Burkert, Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10968
to look at the new patch set (#11).
Change subject: schema: add is_deleted virtual column
......................................................................
schema: add is_deleted virtual column
This patch introduces a very basic concept of a "virtual column". Virtual
columns borrow from other databases in that they are columns that, rather
than being backed by physical data, are instead backed by Kudu itself. They
may not be part of a schema during table creation/alteration, but may be
added to projections during a scan.
Kudu's virtual columns are defined as logical data types. As data types are
not user-defined, there's no danger of a "collision" between a virtual
column and a physical column as there would be if a virtual column occupied
a well-defined name.
A Kudu subsystem on the scan path that wishes to interact with a virtual
column needs to first figure out if the projection includes it. When
projected, the virtual column's data will be either some default or null
(depending on exactly how it was defined in the projection); it's the
subsystem's responsibility to fill in something meaningful afterwards.
Beyond the basic definition, this patch introduces an IS_DELETED virtual
column derived from BOOL. IS_DELETED will be used to support incremental
backups by describing whether a row was deleted between two timestamps.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/common.proto
M src/kudu/common/schema.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
6 files changed, 119 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/11
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 11
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 15: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 15
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 06 Aug 2018 20:25:40 +0000
Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 7:
(1 comment)
I'm curious how this will plumb through all the way to the client. Have you started working on any such patches for TabletService, etc?
http://gerrit.cloudera.org:8080/#/c/10968/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/10968/7/src/kudu/master/catalog_manager.cc@1302
PS7, Line 1302: if (ContainsKey(Schema::kVirtualColumnNames, col_name)) {
Instead of the set, maybe we should just reserve all identifiers of the form $$*$$? Or if we are afraid someone already named such a column, $$kudu.*$$?
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Jul 2018 22:50:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 13: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 13
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Aug 2018 00:16:38 +0000
Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 12:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@158
PS11, Line 158: static bool IsVirtual() {
: return false;
: }
> nit: maybe, move this into the DataTypeTraits template itself and have cust
Is that possible? I thought the DataTypeTraits template was intentionally empty; if you look at some of the other specializations, there's what appears to be intentional repetition of implementations (i.e. min_value() and max_value()).
I just tried it out: I added an IsVirtual static function to the DataTypeTraits template, removed it from the specializations, but kept it in DerivedTypeTraits and DataTypeTraits<IS_DELETED>. I got the following compilation failure:
FAILED: src/kudu/common/CMakeFiles/kudu_common.dir/types.cc.o
/usr/lib/ccache/c++ -DHAVE_LIB_VMEM=1 -DKUDU_HEADERS_NO_STUBS=1 -DKUDU_HEADERS_USE_RICH_SLICE=1 -DKUDU_HEADERS_USE_SHORT_STATUS_MACROS=1 -DKUDU_STATIC_DEFINE -DTCMALLOC_ENABLED -D__STDC_FORMAT_MACROS -Dkudu_common_EXPORTS -Isrc -I../../src -isystem ../../thirdparty/installed/common/include -isystem ../../thirdparty/installed/uninstrumented/include -msse4.2 -Wall -Wno-sign-compare -Wno-comment -Wno-deprecated -pthread -fno-strict-aliasing -DBOOST_DATE_TIME_POSIX_TIME_STD_CONFIG -ggdb -std=c++11 -fuse-ld=gold -fsized-deallocation -g -fPIC -fPIC -MD -MT src/kudu/common/CMakeFiles/kudu_common.dir/types.cc.o -MF src/kudu/common/CMakeFiles/kudu_common.dir/types.cc.o.d -o src/kudu/common/CMakeFiles/kudu_common.dir/types.cc.o -c ../../src/kudu/common/types.cc
../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)0>]’:
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)0]’
../../src/kudu/common/types.cc:73:23: required from here
../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)0>’
is_virtual_(TypeTraitsClass::IsVirtual()),
~~~~~~~~~~~~~~~~~~~~~~~~~~^~
../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)1>]’:
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)1]’
../../src/kudu/common/types.cc:74:22: required from here
../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)1>’
../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)2>]’:
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)2]’
../../src/kudu/common/types.cc:75:24: required from here
../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)2>’
../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)3>]’:
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)3]’
../../src/kudu/common/types.cc:76:23: required from here
../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)3>’
../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)4>]’:
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)4]’
../../src/kudu/common/types.cc:77:24: required from here
../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)4>’
../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)5>]’:
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)5]’
../../src/kudu/common/types.cc:78:23: required from here
../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)5>’
../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)6>]’:
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)6]’
../../src/kudu/common/types.cc:79:24: required from here
../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)6>’
../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)7>]’:
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)7]’
../../src/kudu/common/types.cc:80:23: required from here
../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)7>’
../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)9>]’:
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)9]’
../../src/kudu/common/types.cc:83:22: required from here
../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)9>’
../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)10>]’:
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)10]’
../../src/kudu/common/types.cc:84:23: required from here
../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)10>’
../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)11>]’:
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)11]’
../../src/kudu/common/types.cc:85:24: required from here
../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)11>’
../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)12>]’:
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)12]’
../../src/kudu/common/types.cc:86:24: required from here
../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)12>’
../../src/kudu/common/types.cc: In instantiation of ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)14>]’:
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)14]’
../../src/kudu/common/types.cc:87:24: required from here
../../src/kudu/common/types.cc:40:43: error: ‘IsVirtual’ is not a member of ‘kudu::TypeTraits<(kudu::DataType)14>’
In file included from ../../src/kudu/common/types.cc:18:0:
../../src/kudu/common/types.h: In instantiation of ‘static bool kudu::DerivedTypeTraits<PhysicalType>::IsVirtual() [with kudu::DataType PhysicalType = (kudu::DataType)7]’:
../../src/kudu/common/types.cc:40:43: required from ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)13>]’
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)13]’
../../src/kudu/common/types.cc:81:33: required from here
../../src/kudu/common/types.h:497:51: error: ‘IsVirtual’ is not a member of ‘kudu::DataTypeTraits<(kudu::DataType)7>’
return DataTypeTraits<PhysicalType>::IsVirtual();
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
../../src/kudu/common/types.h: In instantiation of ‘static bool kudu::DerivedTypeTraits<PhysicalType>::IsVirtual() [with kudu::DataType PhysicalType = (kudu::DataType)12]’:
../../src/kudu/common/types.cc:40:43: required from ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)8>]’
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)8]’
../../src/kudu/common/types.cc:82:24: required from here
../../src/kudu/common/types.h:497:51: error: ‘IsVirtual’ is not a member of ‘kudu::DataTypeTraits<(kudu::DataType)12>’
../../src/kudu/common/types.h: In instantiation of ‘static bool kudu::DerivedTypeTraits<PhysicalType>::IsVirtual() [with kudu::DataType PhysicalType = (kudu::DataType)5]’:
../../src/kudu/common/types.cc:40:43: required from ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)15>]’
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)15]’
../../src/kudu/common/types.cc:88:27: required from here
../../src/kudu/common/types.h:497:51: error: ‘IsVirtual’ is not a member of ‘kudu::DataTypeTraits<(kudu::DataType)5>’
../../src/kudu/common/types.h: In instantiation of ‘static bool kudu::DerivedTypeTraits<PhysicalType>::IsVirtual() [with kudu::DataType PhysicalType = (kudu::DataType)14]’:
../../src/kudu/common/types.cc:40:43: required from ‘kudu::TypeInfo::TypeInfo(TypeTraitsClass) [with Type = kudu::TypeTraits<(kudu::DataType)17>]’
../../src/kudu/common/types.cc:96:49: required from ‘void kudu::TypeInfoResolver::AddMapping() [with kudu::DataType type = (kudu::DataType)17]’
../../src/kudu/common/types.cc:90:28: required from here
../../src/kudu/common/types.h:497:51: error: ‘IsVirtual’ is not a member of ‘kudu::DataTypeTraits<(kudu::DataType)14>’
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@673
PS11, Line 673: case IS_DELETED:
> nit: add FALLTHROUGH_INTENDED ?
Done
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@763
PS11, Line 763: // Examples:
> nit: add FALLTHROUGH_INTENDED ?
Done
http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/10968/10/src/kudu/common/types.h@625
PS10, Line 625: return DataTypeTraits<BOOL>::name();
> But if it's name is 'bool', then how to distinguish between a column of vir
It's worth adding that in normal scan operations users wouldn't necessarily see what I saw; I was working within a unit test that dumped rows. I don't feel strongly about it, so if everyone else thinks IS_DELETED (or something like it) is a better type name, I'll change it.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 12
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 02 Aug 2018 18:45:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 14: Verified+1
Posted http://gerrit.cloudera.org:8080/11125 for the unrelated test failure.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 14
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 04 Aug 2018 21:50:50 +0000
Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10968
to look at the new patch set (#4).
Change subject: schema: add is_deleted virtual column
......................................................................
schema: add is_deleted virtual column
This patch introduces a very basic concept of a "virtual column", defined as
a static column with a name, type, and defaults. A Kudu subsystem that
wishes to interact with a virtual column needs to first figure out if the
projection includes it (via ColumnSchema::Equals()). When projected, the
virtual column's data will be entirely default; it's the subsystem's
responsibility to fill in something meaningful afterwards.
This first cut is hardly robust. It remains to be seen how usable this
bare-bones abstraction will be; so far it's sufficient for the MemRowSet.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
4 files changed, 47 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/4
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Dan Burkert, Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10968
to look at the new patch set (#7).
Change subject: schema: add is_deleted virtual column
......................................................................
schema: add is_deleted virtual column
This patch introduces a very basic concept of a "virtual column", defined as
a static column with a name, type, and defaults. A Kudu subsystem that
wishes to interact with a virtual column needs to first figure out if the
projection includes it (via ColumnSchema::Equals()). When projected, the
virtual column's data will be entirely default; it's the subsystem's
responsibility to fill in something meaningful afterwards.
This first cut is hardly robust. It remains to be seen how usable this
bare-bones abstraction will be; so far it's sufficient for the MemRowSet.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
4 files changed, 53 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/7
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/10968/11/src/kudu/common/types.h@673
PS11, Line 673: case IS_DELETED:
> nit: add FALLTHROUGH_INTENDED ?
In my opinion this isn't an appropriate use for FALLTHRUGH_INTENDED. It should only be used when a case contains an statement, and then after evaluating that statement falls into another case. The docs on FALLTHROUGH_INTENDED show this:
// switch (x) {
// case 40:
// case 41:
// if (truth_is_out_there) {
// ++x;
// FALLTHROUGH_INTENDED; // Use instead of/along with annotations in
// // comments.
// } else {
// return x;
// }
// case 42:
// ...
Notice how there is no FALLTHROUGH_INTENDED annotation for the (statement-less) case 40.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 12
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 02 Aug 2018 18:50:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10968
to look at the new patch set (#2).
Change subject: memrowset: support iteration with is_deleted virtual column
......................................................................
memrowset: support iteration with is_deleted virtual column
This patch rounds out the MemRowSet changes for incremental backups.
Taken together, it is now possible to iterate on a specific time range and
to learn which rows were deleted during that time range.
The patch introduces a very basic concept of a "virtual column", defined as
a statically defined column with a name, type, and defaults. A Kudu
subsystem that wishes to interact with a virtual column needs to first
figure out if the projection includes it (via ColumnSchema::Equals()). When
projected, the virtual column's data will be entirely default; it's the
subsystem's responsibility to fill in something meaningful afterwards.
There's absolutely no robustness here. For example, there's currently
nothing stopping someone from creating a real column with the same name. It
remains to be seen how usable this bare-bones abstraction will be; so far
it's sufficient for the MemRowSet.
Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
---
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
5 files changed, 105 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/2
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] schema: add is deleted virtual column
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
......................................................................
Patch Set 7: Verified+1
Overriding Jenkins, unrelated Java test failures.
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Jul 2018 18:01:05 +0000
Gerrit-HasComments: No