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 00:52:23 UTC
[kudu-CR] bshuf block: some code cleanup
Todd Lipcon has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5193
Change subject: bshuf_block: some code cleanup
......................................................................
bshuf_block: some code cleanup
* Having just had a bug in the indexing of the 'data_' array, I made
that less error-prone by adding a cell_ptr() accessor to the builder.
* Some typo/grammar fixes/reformatting in comments.
* Rename kMaxHeaderSize to kHeaderSize since the header is fixed-length.
Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
2 files changed, 62 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5193/1
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
[kudu-CR] bshuf block: some code cleanup
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: bshuf_block: some code cleanup
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/5193/3/src/kudu/cfile/bshuf_block.cc
File src/kudu/cfile/bshuf_block.cc:
PS3, Line 59: uint32_t value = *cell_ptr(i);
: max_value = std::max(max_value, value);
nit: may be, drop the local variable:
max = std::max(max_value, *cell_ptr(i));
PS3, Line 152: int
hopefully, 'n' cannot be too big
PS3, Line 154: implicit_cast
why cast is needed here at all? This is an unsigned type, so direct assignment would be enough, IMO.
PS3, Line 160: implicit_cast
ditto: I don't think cast is needed here.
http://gerrit.cloudera.org:8080/#/c/5193/3/src/kudu/cfile/bshuf_block.h
File src/kudu/cfile/bshuf_block.h:
PS3, Line 155: int
nit: why int, not uint32_t ?
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
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: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] bshuf block: some code cleanup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: bshuf_block: some code cleanup
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5193/6/src/kudu/cfile/bshuf_block.cc
File src/kudu/cfile/bshuf_block.cc:
Line 22: #include "kudu/gutil/casts.h"
> Don't think this is actually used?
Done
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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] bshuf block: some code cleanup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5193
to look at the new patch set (#7).
Change subject: bshuf_block: some code cleanup
......................................................................
bshuf_block: some code cleanup
* Some typo/grammar fixes/reformatting in comments.
* Rename kMaxHeaderSize to kHeaderSize since the header is fixed-length.
* Adds a private cell_ptr() function instead of error prone indexing
with multiplication into the data_ buffer.
* Remove an unnecessarily UINT32 specialization
* Fix some C-style casts and unnecessary type punning in favor of
memcpy()
* Cleaned up padding code to use KUDU_ALIGN_UP
This also adds a TODO for a bug with GetLastKey: we are mistakenly
indexing into data_[count - 1] instead of data_[(count - 1) *
size_of_type] which causes incorrect results. The fix for this is in the
following commit.
Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
2 files changed, 88 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5193/7
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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>
[kudu-CR] bshuf block: some code cleanup
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: bshuf_block: some code cleanup
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5193/6/src/kudu/cfile/bshuf_block.cc
File src/kudu/cfile/bshuf_block.cc:
PS6, Line 152: uint32_t value = array[i];
: memcpy(&array[i * sizeof(uint32_t)], &value, sizeof(value));
nit: I missed that while doing the review, but memmove() might be a little bit faster:
https://gist.github.com/alexeyserbin/9a1612f13c22d4df1946c8831a8b7910
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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] bshuf block: some code cleanup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: bshuf_block: some code cleanup
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/5193/3/src/kudu/cfile/bshuf_block.cc
File src/kudu/cfile/bshuf_block.cc:
PS3, Line 59: uint32_t value = *cell_ptr(i);
: max_value = std::max(max_value, value);
> nit: may be, drop the local variable:
Done
PS3, Line 152: int
> hopefully, 'n' cannot be too big
yea, we'd never have >2B entries in a block
PS3, Line 154: implicit_cast
> why cast is needed here at all? This is an unsigned type, so direct assign
Done
PS3, Line 160: implicit_cast
> ditto: I don't think cast is needed here.
Done
http://gerrit.cloudera.org:8080/#/c/5193/3/src/kudu/cfile/bshuf_block.h
File src/kudu/cfile/bshuf_block.h:
PS3, Line 155: int
> nit: why int, not uint32_t ?
google style guide says not to use unsigned types except to represent bit patterns or wire formats, etc. A lot of this code is old from before we started following that rule, which is why there's a bit of inconsistency.
Updated this bit of code to be clearer anyway.
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
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: Alexey Serbin <as...@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] bshuf block: some code cleanup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: bshuf_block: some code cleanup
......................................................................
bshuf_block: some code cleanup
* Some typo/grammar fixes/reformatting in comments.
* Rename kMaxHeaderSize to kHeaderSize since the header is fixed-length.
* Adds a private cell_ptr() function instead of error prone indexing
with multiplication into the data_ buffer.
* Remove an unnecessarily UINT32 specialization
* Fix some C-style casts and unnecessary type punning in favor of
memcpy()
* Cleaned up padding code to use KUDU_ALIGN_UP
This also adds a TODO for a bug with GetLastKey: we are mistakenly
indexing into data_[count - 1] instead of data_[(count - 1) *
size_of_type] which causes incorrect results. The fix for this is in the
following commit.
Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Reviewed-on: http://gerrit.cloudera.org:8080/5193
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
2 files changed, 88 insertions(+), 79 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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>
[kudu-CR] bshuf block: some code cleanup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5193
to look at the new patch set (#3).
Change subject: bshuf_block: some code cleanup
......................................................................
bshuf_block: some code cleanup
* Some typo/grammar fixes/reformatting in comments.
* Rename kMaxHeaderSize to kHeaderSize since the header is fixed-length.
* Adds a private cell_ptr() function instead of error prone indexing
with multiplication into the data_ buffer.
* Remove an unnecessarily UINT32 specialization
* Fix some C-style casts and unnecessary type punning in favor of
memcpy()
This partially fixes a bug with GetLastKey: previously we were
mistakenly indexing into data_[count - 1] instead of
data_[(count - 1) * size_of_type] which caused incorrect results.
There is still another bug here in GetLastKey() which will be fixed in
the next commit.
Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
2 files changed, 81 insertions(+), 63 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5193/3
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] bshuf block: some code cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: bshuf_block: some code cleanup
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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: No
[kudu-CR] bshuf block: some code cleanup
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/5193
to look at the new patch set (#4).
Change subject: bshuf_block: some code cleanup
......................................................................
bshuf_block: some code cleanup
* Some typo/grammar fixes/reformatting in comments.
* Rename kMaxHeaderSize to kHeaderSize since the header is fixed-length.
* Adds a private cell_ptr() function instead of error prone indexing
with multiplication into the data_ buffer.
* Remove an unnecessarily UINT32 specialization
* Fix some C-style casts and unnecessary type punning in favor of
memcpy()
This partially fixes a bug with GetLastKey: previously we were
mistakenly indexing into data_[count - 1] instead of
data_[(count - 1) * size_of_type] which caused incorrect results.
There is still another bug here in GetLastKey() which will be fixed in
the next commit.
Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
2 files changed, 82 insertions(+), 62 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5193/4
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] bshuf block: some code cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: bshuf_block: some code cleanup
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5193/6/src/kudu/cfile/bshuf_block.cc
File src/kudu/cfile/bshuf_block.cc:
Line 22: #include "kudu/gutil/casts.h"
Don't think this is actually used?
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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] bshuf block: some code cleanup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5193
to look at the new patch set (#6).
Change subject: bshuf_block: some code cleanup
......................................................................
bshuf_block: some code cleanup
* Some typo/grammar fixes/reformatting in comments.
* Rename kMaxHeaderSize to kHeaderSize since the header is fixed-length.
* Adds a private cell_ptr() function instead of error prone indexing
with multiplication into the data_ buffer.
* Remove an unnecessarily UINT32 specialization
* Fix some C-style casts and unnecessary type punning in favor of
memcpy()
* Cleaned up padding code to use KUDU_ALIGN_UP
This also adds a TODO for a bug with GetLastKey: we are mistakenly
indexing into data_[count - 1] instead of data_[(count - 1) *
size_of_type] which causes incorrect results. The fix for this is in the
following commit.
Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
2 files changed, 88 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5193/6
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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>
[kudu-CR] bshuf block: some code cleanup
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/5193
to look at the new patch set (#5).
Change subject: bshuf_block: some code cleanup
......................................................................
bshuf_block: some code cleanup
* Some typo/grammar fixes/reformatting in comments.
* Rename kMaxHeaderSize to kHeaderSize since the header is fixed-length.
* Adds a private cell_ptr() function instead of error prone indexing
with multiplication into the data_ buffer.
* Remove an unnecessarily UINT32 specialization
* Fix some C-style casts and unnecessary type punning in favor of
memcpy()
This also adds a TODO for a bug with GetLastKey: we are mistakenly
indexing into data_[count - 1] instead of data_[(count - 1) *
size_of_type] which causes incorrect results. The fix for this is in the
following commit.
Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
2 files changed, 82 insertions(+), 62 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5193/5
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 5
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] bshuf block: some code cleanup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: bshuf_block: some code cleanup
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5193/6/src/kudu/cfile/bshuf_block.cc
File src/kudu/cfile/bshuf_block.cc:
PS6, Line 152: uint32_t value = array[i];
: memcpy(&array[i * sizeof(uint32_t)], &value, sizeof(value));
> nit: I missed that while doing the review, but memmove() might be a little
I don't get the same results you did, and also typically memcpy is faster than memmove because it is able to restrict that the src/dst don't overlap. Let's leave as is unless you can show in a macro-benchmark that it matters.
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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] bshuf block: some code cleanup
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: bshuf_block: some code cleanup
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5193/6/src/kudu/cfile/bshuf_block.cc
File src/kudu/cfile/bshuf_block.cc:
PS6, Line 152: uint32_t value = array[i];
: memcpy(&array[i * sizeof(uint32_t)], &value, sizeof(value));
> I don't get the same results you did, and also typically memcpy is faster t
Oops, my bad: the snipped was lacking the actual looping. With that fixed, the results are closer to the expected ones: I updated the gist.
Sorry for the noise.
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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] bshuf block: some code cleanup
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: bshuf_block: some code cleanup
......................................................................
Patch Set 6: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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: No
[kudu-CR] bshuf block: some code cleanup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5193
to look at the new patch set (#2).
Change subject: bshuf_block: some code cleanup
......................................................................
bshuf_block: some code cleanup
* Some typo/grammar fixes/reformatting in comments.
* Rename kMaxHeaderSize to kHeaderSize since the header is fixed-length.
* Adds a private cell_ptr() function instead of error prone indexing
with multiplication into the data_ buffer.
* Remove an unnecessarily UINT32 specialization
* Fix some C-style casts and unnecessary type punning in favor of
memcpy()
This partially fixes a bug with GetLastKey: previously we were
mistakenly indexing into data_[count - 1] instead of
data_[(count - 1) * size_of_type] which caused incorrect results.
There is still another bug here in GetLastKey() which will be fixed in
the next commit.
Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
2 files changed, 80 insertions(+), 62 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5193/2
--
To view, visit http://gerrit.cloudera.org:8080/5193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot