You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Jim Apple (Code Review)" <ge...@cloudera.org> on 2019/02/03 00:40:56 UTC

[Impala-ASF-CR] IMPALA-5031: `uint8 t & int` type is int

Jim Apple has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12346


Change subject: IMPALA-5031: `uint8_t & int` type is int
......................................................................

IMPALA-5031: `uint8_t & int` type is int

UBSAN finds the following in ParquetBoolDecoder.TestDecodeAndSkipping:

util/bit-stream-utils.inline.h:156:25: runtime error: left shift of 42 by 28 places cannot be represented in type 'int'
    #0 BatchedBitReader::GetUleb128Int(unsigned int*) util/bit-stream-utils.inline.h:156:25
    #1 RleBatchDecoder<bool>::NextCounts() util/rle-encoding.h:778:40
    #2 RleBatchDecoder<bool>::NextNumRepeats() util/rle-encoding.h:622:28
    #3 RleBatchDecoder<bool>::GetValues(int, bool*) util/rle-encoding.h:858:27
    #4 bool ParquetBoolDecoder::DecodeValue<(parquet::Encoding::type)3>(bool*) exec/parquet/parquet-bool-decoder.h:85:24
    #5 TestSkipping(parquet::Encoding::type, unsigned char*, int, vector<bool> const&, int, int)::$_0::operator()() const exec/parquet/parquet-bool-decoder-test.cc:59
    #6 TestSkipping(parquet::Encoding::type, unsigned char*, int, vector<bool> const&, int, int) exec/parquet/parquet-bool-decoder-test.cc:69:221
    #7 ParquetBoolDecoder_TestDecodeAndSkipping_Test::TestBody() exec/parquet/parquet-bool-decoder-test.cc:85:5
    #9 testing::Test::Run() (/home/ubuntu/Impala/be/build/debug/exec/parquet/parquet-bool-decoder-test+0x6ee4f09)

The problem is the line

    *v |= (byte & 0x7F) << shift;

byte is an uint8_t and 0x7F is an int. The standard section
[expr.bit.and] then applies the "usual arithmetic conversions"
specified in [expr], which applies "if the type of the operand with
signed integer type can represent all of the values of the type of the
operand with unsigned integer type, the operand with unsigned integer
type shall be converted to the type of the operand with signed integer
type." That makes byte & 0x7F a signed integer type, and [expr.shift]
says that "if E1 has a signed type and non-negative value, and E1×2^E2
is representable in the corresponding unsigned type of the result
type, then that value, converted to the result type, is the resulting
value; otherwise, the behavior is undefined."

Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
---
M be/src/util/bit-stream-utils.inline.h
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/12346/1
-- 
To view, visit http://gerrit.cloudera.org:8080/12346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Gerrit-Change-Number: 12346
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>

[Impala-ASF-CR] IMPALA-5031: `uint8 t & int` type is int

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12346 )

Change subject: IMPALA-5031: `uint8_t & int` type is int
......................................................................


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3750/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Gerrit-Change-Number: 12346
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 09 Feb 2019 14:35:06 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: `uint8 t & int` type is int

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Tim Armstrong, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-5031: `uint8_t & int` type is int
......................................................................

IMPALA-5031: `uint8_t & int` type is int

UBSAN finds the following in ParquetBoolDecoder.TestDecodeAndSkipping:

util/bit-stream-utils.inline.h:156:25: runtime error: left shift of 42 by 28 places cannot be represented in type 'int'
    #0 BatchedBitReader::GetUleb128Int(unsigned int*) util/bit-stream-utils.inline.h:156:25
    #1 RleBatchDecoder<bool>::NextCounts() util/rle-encoding.h:778:40
    #2 RleBatchDecoder<bool>::NextNumRepeats() util/rle-encoding.h:622:28
    #3 RleBatchDecoder<bool>::GetValues(int, bool*) util/rle-encoding.h:858:27
    #4 bool ParquetBoolDecoder::DecodeValue<(parquet::Encoding::type)3>(bool*) exec/parquet/parquet-bool-decoder.h:85:24
    #5 TestSkipping(parquet::Encoding::type, unsigned char*, int, vector<bool> const&, int, int)::$_0::operator()() const exec/parquet/parquet-bool-decoder-test.cc:59
    #6 TestSkipping(parquet::Encoding::type, unsigned char*, int, vector<bool> const&, int, int) exec/parquet/parquet-bool-decoder-test.cc:69:221
    #7 ParquetBoolDecoder_TestDecodeAndSkipping_Test::TestBody() exec/parquet/parquet-bool-decoder-test.cc:85:5
    #9 testing::Test::Run() (/home/ubuntu/Impala/be/build/debug/exec/parquet/parquet-bool-decoder-test+0x6ee4f09)

The problem is the line

    *v |= (byte & 0x7F) << shift;

byte is an uint8_t and 0x7F is an int. The standard section
[expr.bit.and] then applies the "usual arithmetic conversions"
specified in [expr], which applies "if the type of the operand with
signed integer type can represent all of the values of the type of the
operand with unsigned integer type, the operand with unsigned integer
type shall be converted to the type of the operand with signed integer
type." That makes byte & 0x7F a signed integer type, and [expr.shift]
says that "if E1 has a signed type and non-negative value, and E1×2^E2
is representable in the corresponding unsigned type of the result
type, then that value, converted to the result type, is the resulting
value; otherwise, the behavior is undefined."

Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
---
M be/src/util/bit-stream-utils.inline.h
1 file changed, 4 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/12346/2
-- 
To view, visit http://gerrit.cloudera.org:8080/12346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Gerrit-Change-Number: 12346
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[Impala-ASF-CR] IMPALA-5031: `uint8 t & int` type is int

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12346 )

Change subject: IMPALA-5031: `uint8_t & int` type is int
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1973/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Gerrit-Change-Number: 12346
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 03 Feb 2019 01:23:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: `uint8 t & int` type is int

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12346 )

Change subject: IMPALA-5031: `uint8_t & int` type is int
......................................................................


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12346/1/be/src/util/bit-stream-utils.inline.h
File be/src/util/bit-stream-utils.inline.h:

http://gerrit.cloudera.org:8080/#/c/12346/1/be/src/util/bit-stream-utils.inline.h@156
PS1, Line 156:     *v |= (byte & 0x7Fu) << shift;
Might be worth leaving a comment here warning of the subtlety.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Gerrit-Change-Number: 12346
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 03 Feb 2019 09:33:53 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5031: `uint8 t & int` type is int

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12346 )

Change subject: IMPALA-5031: `uint8_t & int` type is int
......................................................................


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Gerrit-Change-Number: 12346
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 09 Feb 2019 18:52:33 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: `uint8 t & int` type is int

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12346 )

Change subject: IMPALA-5031: `uint8_t & int` type is int
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12346/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12346/1//COMMIT_MSG@9
PS1, Line 9: UBSAN finds the following in ParquetBoolDecoder.TestDecodeAndSkipping:
> I suspect the before and after versions compile to exactly the same code so
ah, indeed. I checked on the compiler explorer to see that you're right, it's the same resulting code: https://godbolt.org/z/Xc893z

I misunderstood the wording of the standard below.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Gerrit-Change-Number: 12346
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 05 Feb 2019 16:56:50 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5031: `uint8 t & int` type is int

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12346 )

Change subject: IMPALA-5031: `uint8_t & int` type is int
......................................................................

IMPALA-5031: `uint8_t & int` type is int

UBSAN finds the following in ParquetBoolDecoder.TestDecodeAndSkipping:

util/bit-stream-utils.inline.h:156:25: runtime error: left shift of 42 by 28 places cannot be represented in type 'int'
    #0 BatchedBitReader::GetUleb128Int(unsigned int*) util/bit-stream-utils.inline.h:156:25
    #1 RleBatchDecoder<bool>::NextCounts() util/rle-encoding.h:778:40
    #2 RleBatchDecoder<bool>::NextNumRepeats() util/rle-encoding.h:622:28
    #3 RleBatchDecoder<bool>::GetValues(int, bool*) util/rle-encoding.h:858:27
    #4 bool ParquetBoolDecoder::DecodeValue<(parquet::Encoding::type)3>(bool*) exec/parquet/parquet-bool-decoder.h:85:24
    #5 TestSkipping(parquet::Encoding::type, unsigned char*, int, vector<bool> const&, int, int)::$_0::operator()() const exec/parquet/parquet-bool-decoder-test.cc:59
    #6 TestSkipping(parquet::Encoding::type, unsigned char*, int, vector<bool> const&, int, int) exec/parquet/parquet-bool-decoder-test.cc:69:221
    #7 ParquetBoolDecoder_TestDecodeAndSkipping_Test::TestBody() exec/parquet/parquet-bool-decoder-test.cc:85:5
    #9 testing::Test::Run() (/home/ubuntu/Impala/be/build/debug/exec/parquet/parquet-bool-decoder-test+0x6ee4f09)

The problem is the line

    *v |= (byte & 0x7F) << shift;

byte is an uint8_t and 0x7F is an int. The standard section
[expr.bit.and] then applies the "usual arithmetic conversions"
specified in [expr], which applies "if the type of the operand with
signed integer type can represent all of the values of the type of the
operand with unsigned integer type, the operand with unsigned integer
type shall be converted to the type of the operand with signed integer
type." That makes byte & 0x7F a signed integer type, and [expr.shift]
says that "if E1 has a signed type and non-negative value, and E1×2^E2
is representable in the corresponding unsigned type of the result
type, then that value, converted to the result type, is the resulting
value; otherwise, the behavior is undefined."

Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Reviewed-on: http://gerrit.cloudera.org:8080/12346
Reviewed-by: Jim Apple <jb...@apache.org>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/util/bit-stream-utils.inline.h
1 file changed, 4 insertions(+), 1 deletion(-)

Approvals:
  Jim Apple: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Gerrit-Change-Number: 12346
Gerrit-PatchSet: 4
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[Impala-ASF-CR] IMPALA-5031: `uint8 t & int` type is int

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12346 )

Change subject: IMPALA-5031: `uint8_t & int` type is int
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2062/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Gerrit-Change-Number: 12346
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 09 Feb 2019 15:13:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: `uint8 t & int` type is int

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12346 )

Change subject: IMPALA-5031: `uint8_t & int` type is int
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12346/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12346/1//COMMIT_MSG@9
PS1, Line 9: UBSAN finds the following in ParquetBoolDecoder.TestDecodeAndSkipping:
since this looks like an actual bug, are we currently subject to wrong results here?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Gerrit-Change-Number: 12346
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 04 Feb 2019 19:18:05 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5031: `uint8 t & int` type is int

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/12346 )

Change subject: IMPALA-5031: `uint8_t & int` type is int
......................................................................


Patch Set 3: Code-Review+2

rebase carry Tim's +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Gerrit-Change-Number: 12346
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 09 Feb 2019 14:34:33 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: `uint8 t & int` type is int

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12346 )

Change subject: IMPALA-5031: `uint8_t & int` type is int
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12346/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12346/1//COMMIT_MSG@9
PS1, Line 9: UBSAN finds the following in ParquetBoolDecoder.TestDecodeAndSkipping:
> since this looks like an actual bug, are we currently subject to wrong resu
I suspect the before and after versions compile to exactly the same code so we're probably OK in practice. It looks like we're exercising the code in a unit test that verifies results so should probably be ok.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Gerrit-Change-Number: 12346
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 05 Feb 2019 09:50:28 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5031: `uint8 t & int` type is int

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/12346 )

Change subject: IMPALA-5031: `uint8_t & int` type is int
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

carry Tim's +2

http://gerrit.cloudera.org:8080/#/c/12346/1/be/src/util/bit-stream-utils.inline.h
File be/src/util/bit-stream-utils.inline.h:

http://gerrit.cloudera.org:8080/#/c/12346/1/be/src/util/bit-stream-utils.inline.h@156
PS1, Line 156:     // The constant below must be explicitly unsigned to ensure that the result of the
> Might be worth leaving a comment here warning of the subtlety.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6e0b956751090f3f8aadd6783b5e06e55e57abe
Gerrit-Change-Number: 12346
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 09 Feb 2019 14:34:16 +0000
Gerrit-HasComments: Yes