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

[kudu-CR] encodings: change IsBlockFull() to not take a limit parameter 

Todd Lipcon has uploaded a new change for review.

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

Change subject: encodings: change IsBlockFull() to not take a limit parameter
......................................................................

encodings: change IsBlockFull() to not take a limit parameter

Everywhere we called this, we used the same value (the configured cfile
block size). Given that, it's simpler to not take the parameter. This
makes it easier for an encoder to determine whether or not it is full in
some other context, since it can safely assume that it knows the block
size up front.

Change-Id: I4598c1555732649eeebf43231082dd641ecfc576
---
M src/kudu/cfile/binary_dict_block.cc
M src/kudu/cfile/binary_dict_block.h
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/cfile/binary_plain_block.h
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/binary_prefix_block.h
M src/kudu/cfile/block_encodings.h
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/gvint_block.cc
M src/kudu/cfile/gvint_block.h
M src/kudu/cfile/plain_bitmap_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/cfile/type_encodings.cc
16 files changed, 53 insertions(+), 43 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4598c1555732649eeebf43231082dd641ecfc576
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>

[kudu-CR] encodings: change IsBlockFull() to not take a limit parameter

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

Change subject: encodings: change IsBlockFull() to not take a limit parameter
......................................................................


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4598c1555732649eeebf43231082dd641ecfc576
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] encodings: change IsBlockFull() to not take a limit parameter

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

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

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

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

Change subject: encodings: change IsBlockFull() to not take a limit parameter
......................................................................

encodings: change IsBlockFull() to not take a limit parameter

Everywhere we called this, we used the same value (the configured cfile
block size). Given that, it's simpler to not take the parameter. This
makes it easier for an encoder to determine whether or not it is full in
some other context, since it can safely assume that it knows the block
size up front.

Change-Id: I4598c1555732649eeebf43231082dd641ecfc576
---
M src/kudu/cfile/binary_dict_block.cc
M src/kudu/cfile/binary_dict_block.h
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/cfile/binary_plain_block.h
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/binary_prefix_block.h
M src/kudu/cfile/block_encodings.h
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/gvint_block.cc
M src/kudu/cfile/gvint_block.h
M src/kudu/cfile/plain_bitmap_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/cfile/type_encodings.cc
16 files changed, 53 insertions(+), 43 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4598c1555732649eeebf43231082dd641ecfc576
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] encodings: change IsBlockFull() to not take a limit parameter

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

Change subject: encodings: change IsBlockFull() to not take a limit parameter
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/5200/2/src/kudu/cfile/binary_plain_block.cc
File src/kudu/cfile/binary_plain_block.cc:

Line 86:     // TODO: does it cost a lot to account these things specifically?
> warning: missing username/bug in TODO [google-readability-todo]
Done


http://gerrit.cloudera.org:8080/#/c/5200/2/src/kudu/cfile/binary_prefix_block.cc
File src/kudu/cfile/binary_prefix_block.cc:

Line 83:   // TODO: take restarts size into account
> warning: missing username/bug in TODO [google-readability-todo]
Done


http://gerrit.cloudera.org:8080/#/c/5200/2/src/kudu/cfile/gvint_block.h
File src/kudu/cfile/gvint_block.h:

Line 52:   int Add(const uint8_t *vals, size_t count) OVERRIDE;
> warning: function 'kudu::cfile::GVIntBlockBuilder::Add' has a definition wi
Done


http://gerrit.cloudera.org:8080/#/c/5200/2/src/kudu/cfile/plain_bitmap_block.h
File src/kudu/cfile/plain_bitmap_block.h:

Line 52:     return writer_.bytes_written() > options_->storage_attributes.cfile_block_size;
> error: member access into incomplete type 'const kudu::cfile::WriterOptions
Done


http://gerrit.cloudera.org:8080/#/c/5200/2/src/kudu/cfile/rle_block.h
File src/kudu/cfile/rle_block.h:

Line 69:     return encoder_.len() > options_->storage_attributes.cfile_block_size;
> error: member access into incomplete type 'const kudu::cfile::WriterOptions
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4598c1555732649eeebf43231082dd641ecfc576
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] encodings: change IsBlockFull() to not take a limit parameter

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

Change subject: encodings: change IsBlockFull() to not take a limit parameter
......................................................................


encodings: change IsBlockFull() to not take a limit parameter

Everywhere we called this, we used the same value (the configured cfile
block size). Given that, it's simpler to not take the parameter. This
makes it easier for an encoder to determine whether or not it is full in
some other context, since it can safely assume that it knows the block
size up front.

Change-Id: I4598c1555732649eeebf43231082dd641ecfc576
Reviewed-on: http://gerrit.cloudera.org:8080/5200
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/cfile/binary_dict_block.cc
M src/kudu/cfile/binary_dict_block.h
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/cfile/binary_plain_block.h
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/binary_prefix_block.h
M src/kudu/cfile/block_encodings.h
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/gvint_block.cc
M src/kudu/cfile/gvint_block.h
M src/kudu/cfile/plain_bitmap_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/cfile/type_encodings.cc
16 files changed, 60 insertions(+), 48 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I4598c1555732649eeebf43231082dd641ecfc576
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] encodings: change IsBlockFull() to not take a limit parameter

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

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

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

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

Change subject: encodings: change IsBlockFull() to not take a limit parameter
......................................................................

encodings: change IsBlockFull() to not take a limit parameter

Everywhere we called this, we used the same value (the configured cfile
block size). Given that, it's simpler to not take the parameter. This
makes it easier for an encoder to determine whether or not it is full in
some other context, since it can safely assume that it knows the block
size up front.

Change-Id: I4598c1555732649eeebf43231082dd641ecfc576
---
M src/kudu/cfile/binary_dict_block.cc
M src/kudu/cfile/binary_dict_block.h
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/cfile/binary_plain_block.h
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/binary_prefix_block.h
M src/kudu/cfile/block_encodings.h
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/gvint_block.cc
M src/kudu/cfile/gvint_block.h
M src/kudu/cfile/plain_bitmap_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/cfile/type_encodings.cc
16 files changed, 60 insertions(+), 48 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4598c1555732649eeebf43231082dd641ecfc576
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>