You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "KeDeng (Code Review)" <ge...@cloudera.org> on 2025/06/20 04:44:38 UTC
[kudu-CR] [util] eliminate compiler warnings in RLE encoding debug checks
KeDeng has uploaded this change for review. ( http://gerrit.cloudera.org:8080/23055
Change subject: [util] eliminate compiler warnings in RLE encoding debug checks
......................................................................
[util] eliminate compiler warnings in RLE encoding debug checks
This patch resolves two categories of compiler warnings in RLE encoding
that were discovered during Rocky Linux compilation and also exist in CentOS:
1. Boolean comparison warnings (-Wbool-compare):
For T=bool and BIT_WIDTH=1, the debug check "value < (1LL << BIT_WIDTH)"
(which evaluates to "value < 2") is always true for boolean values (0 or 1),
triggering warnings like:
`
/data/code/kudu/src/kudu/util/rle-encoding.h:388:37:
warning: comparison of constant '2' with boolean expression is always true
[-Wbool-compare]
DCHECK(value < (1LL << BIT_WIDTH));
~~~~~^~~~~~~~~~~~~~~~~~~~
`
2. Shift overflow warnings (-Wshift-count-overflow):
For BIT_WIDTH=64, the expression (1LL << 64) causes undefined behavior
by shifting beyond long long's bit width, triggering warnings like:
`
/data/code/kudu/src/kudu/util/rle-encoding.h:388:44:
warning: left shift count >= width of type [-Wshift-count-overflow]
DCHECK(value < (1LL << BIT_WIDTH));
~~~~^~~~~~~~~~~~~
`
Implemented solution:
- Added template specialization for validation:
* For boolean types: Strictly validates values are 0 or 1
* For other types: Maintains original range check while avoiding
unsafe 64-bit shifts
- Created helper template struct ValueCheck with:
* Boolean specialization: Uses DCHECK(value == 0 || value == 1)
* Non-boolean specialization: Uses DCHECK(value < (1LL << BW))
with guard for BW < 64
- Preserved all original safety checks while eliminating warnings
The changes:
- Completely separate code paths for boolean/non-boolean types
- Prevent generation of problematic comparisons for booleans
- Avoid undefined shift behavior for 64-bit widths
- Maintain identical safety guarantees
Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
---
M src/kudu/util/rle-encoding.h
1 file changed, 25 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/23055/1
--
To view, visit http://gerrit.cloudera.org:8080/23055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Gerrit-Change-Number: 23055
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>
[kudu-CR] [util] eliminate compiler warnings in RLE encoding debug checks
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/23055 )
Change subject: [util] eliminate compiler warnings in RLE encoding debug checks
......................................................................
[util] eliminate compiler warnings in RLE encoding debug checks
This patch resolves two categories of compiler warnings in RLE encoding
that were discovered during Rocky Linux compilation and also exist in CentOS:
1. Boolean comparison warnings (-Wbool-compare):
For T=bool and BIT_WIDTH=1, the debug check "value < (1LL << BIT_WIDTH)"
(which evaluates to "value < 2") is always true for boolean values (0 or 1),
triggering warnings like:
`
/data/code/kudu/src/kudu/util/rle-encoding.h:388:37:
warning: comparison of constant '2' with boolean expression is always true
[-Wbool-compare]
DCHECK(value < (1LL << BIT_WIDTH));
~~~~~^~~~~~~~~~~~~~~~~~~~
`
2. Shift overflow warnings (-Wshift-count-overflow):
For BIT_WIDTH=64, the expression (1LL << 64) causes undefined behavior
by shifting beyond long long's bit width, triggering warnings like:
`
/data/code/kudu/src/kudu/util/rle-encoding.h:388:44:
warning: left shift count >= width of type [-Wshift-count-overflow]
DCHECK(value < (1LL << BIT_WIDTH));
~~~~^~~~~~~~~~~~~
`
Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Reviewed-on: http://gerrit.cloudera.org:8080/23055
Tested-by: Alexey Serbin <al...@apache.org>
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/util/rle-encoding.h
1 file changed, 5 insertions(+), 1 deletion(-)
Approvals:
Alexey Serbin: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/23055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Gerrit-Change-Number: 23055
Gerrit-PatchSet: 3
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] eliminate compiler warnings in RLE encoding debug checks
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/23055 )
Change subject: [util] eliminate compiler warnings in RLE encoding debug checks
......................................................................
Patch Set 2: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/1478/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/23055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Gerrit-Change-Number: 23055
Gerrit-PatchSet: 2
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 20 Jun 2025 11:14:45 +0000
Gerrit-HasComments: No
[kudu-CR] [util] eliminate compiler warnings in RLE encoding debug checks
Posted by "KeDeng (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/23055
to look at the new patch set (#2).
Change subject: [util] eliminate compiler warnings in RLE encoding debug checks
......................................................................
[util] eliminate compiler warnings in RLE encoding debug checks
This patch resolves two categories of compiler warnings in RLE encoding
that were discovered during Rocky Linux compilation and also exist in CentOS:
1. Boolean comparison warnings (-Wbool-compare):
For T=bool and BIT_WIDTH=1, the debug check "value < (1LL << BIT_WIDTH)"
(which evaluates to "value < 2") is always true for boolean values (0 or 1),
triggering warnings like:
`
/data/code/kudu/src/kudu/util/rle-encoding.h:388:37:
warning: comparison of constant '2' with boolean expression is always true
[-Wbool-compare]
DCHECK(value < (1LL << BIT_WIDTH));
~~~~~^~~~~~~~~~~~~~~~~~~~
`
2. Shift overflow warnings (-Wshift-count-overflow):
For BIT_WIDTH=64, the expression (1LL << 64) causes undefined behavior
by shifting beyond long long's bit width, triggering warnings like:
`
/data/code/kudu/src/kudu/util/rle-encoding.h:388:44:
warning: left shift count >= width of type [-Wshift-count-overflow]
DCHECK(value < (1LL << BIT_WIDTH));
~~~~^~~~~~~~~~~~~
`
Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
---
M src/kudu/util/rle-encoding.h
1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/23055/2
--
To view, visit http://gerrit.cloudera.org:8080/23055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Gerrit-Change-Number: 23055
Gerrit-PatchSet: 2
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] eliminate compiler warnings in RLE encoding debug checks
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [util] eliminate compiler warnings in RLE encoding debug checks
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/23055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Gerrit-Change-Number: 23055
Gerrit-PatchSet: 2
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] eliminate compiler warnings in RLE encoding debug checks
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/23055 )
Change subject: [util] eliminate compiler warnings in RLE encoding debug checks
......................................................................
Patch Set 2:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1478/
--
To view, visit http://gerrit.cloudera.org:8080/23055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Gerrit-Change-Number: 23055
Gerrit-PatchSet: 2
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 20 Jun 2025 09:10:46 +0000
Gerrit-HasComments: No
[kudu-CR] [util] eliminate compiler warnings in RLE encoding debug checks
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/23055 )
Change subject: [util] eliminate compiler warnings in RLE encoding debug checks
......................................................................
Patch Set 1:
Build Aborted
http://jenkins.kudu.apache.org/job/pre_commit/1475/ : ABORTED
--
To view, visit http://gerrit.cloudera.org:8080/23055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Gerrit-Change-Number: 23055
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 20 Jun 2025 09:10:40 +0000
Gerrit-HasComments: No
[kudu-CR] [util] eliminate compiler warnings in RLE encoding debug checks
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23055 )
Change subject: [util] eliminate compiler warnings in RLE encoding debug checks
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/23055/1/src/kudu/util/rle-encoding.h
File src/kudu/util/rle-encoding.h:
http://gerrit.cloudera.org:8080/#/c/23055/1/src/kudu/util/rle-encoding.h@409
PS1, Line 409: ValueCheck<T, BIT_WIDTH>::Check(value);
> I'd rather think of doing this using constexpr. It's easier to read and it
Regarding the extra check in the RleEncoder's constructor, it should have been the following instead:
if constexpr (std::is_same<T, bool>::value) {
static_assert(BIT_WIDTH == 1);
}
--
To view, visit http://gerrit.cloudera.org:8080/23055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Gerrit-Change-Number: 23055
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 20 Jun 2025 07:38:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] eliminate compiler warnings in RLE encoding debug checks
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/23055 )
Change subject: [util] eliminate compiler warnings in RLE encoding debug checks
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/23055/1/src/kudu/util/rle-encoding.h
File src/kudu/util/rle-encoding.h:
http://gerrit.cloudera.org:8080/#/c/23055/1/src/kudu/util/rle-encoding.h@409
PS1, Line 409: current_value_ = value;
> Regarding the extra check in the RleEncoder's constructor, it should have b
Done
http://gerrit.cloudera.org:8080/#/c/23055/1/src/kudu/util/rle-encoding.h@409
PS1, Line 409: current_value_ = value;
> I'd rather think of doing this using constexpr. It's easier to read and it
Done
--
To view, visit http://gerrit.cloudera.org:8080/23055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Gerrit-Change-Number: 23055
Gerrit-PatchSet: 2
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 20 Jun 2025 09:11:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] eliminate compiler warnings in RLE encoding debug checks
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23055 )
Change subject: [util] eliminate compiler warnings in RLE encoding debug checks
......................................................................
Patch Set 2: Code-Review+2
Thank you for fixing this!
--
To view, visit http://gerrit.cloudera.org:8080/23055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Gerrit-Change-Number: 23055
Gerrit-PatchSet: 2
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 21 Jun 2025 06:46:22 +0000
Gerrit-HasComments: No
[kudu-CR] [util] eliminate compiler warnings in RLE encoding debug checks
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/23055 )
Change subject: [util] eliminate compiler warnings in RLE encoding debug checks
......................................................................
Patch Set 1:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1475/
--
To view, visit http://gerrit.cloudera.org:8080/23055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Gerrit-Change-Number: 23055
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 20 Jun 2025 04:44:45 +0000
Gerrit-HasComments: No
[kudu-CR] [util] eliminate compiler warnings in RLE encoding debug checks
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23055 )
Change subject: [util] eliminate compiler warnings in RLE encoding debug checks
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/23055/1/src/kudu/util/rle-encoding.h
File src/kudu/util/rle-encoding.h:
http://gerrit.cloudera.org:8080/#/c/23055/1/src/kudu/util/rle-encoding.h@409
PS1, Line 409: ValueCheck<T, BIT_WIDTH>::Check(value);
I'd rather think of doing this using constexpr. It's easier to read and it's always under DCHECK() at the top level:
if constexpr (BIT_WIDTH < 64 && BIT_WIDTH > 1) {
DCHECK(value < (1LL << BIT_WIDTH));
}
To address BIT_WIDTH == 1 and T == bool case, one could also add one extra static_assert() into the constructor:
static_assert(BIT_WIDTH > 1 ||
(BIT_WIDTH == 1 && std::is_same<T, bool>::value));
--
To view, visit http://gerrit.cloudera.org:8080/23055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Gerrit-Change-Number: 23055
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 20 Jun 2025 07:21:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] eliminate compiler warnings in RLE encoding debug checks
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23055 )
Change subject: [util] eliminate compiler warnings in RLE encoding debug checks
......................................................................
Patch Set 2: Verified+1
TSAN: unrelated test failure
--
To view, visit http://gerrit.cloudera.org:8080/23055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I230fb92acb18aeda29766f2f131beac9eca8dab9
Gerrit-Change-Number: 23055
Gerrit-PatchSet: 2
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 21 Jun 2025 06:43:31 +0000
Gerrit-HasComments: No