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 2016/11/08 19:36:03 UTC
[Impala-ASF-CR] IMPALA-4433: Fix undefined NDV calculations
Jim Apple has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5004
Change subject: IMPALA-4433: Fix undefined NDV calculations
......................................................................
IMPALA-4433: Fix undefined NDV calculations
GCC's __builtin_ctz[l[l]] functions return undefined results when the
argument is 0. This patch handles that 0 case.
This changes the metadata for the test data, because ctz is used in
HLL++, which is used as the NDV computation in compute stats.
Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
---
M be/src/exprs/aggregate-functions-ir.cc
M be/src/udf_samples/hyperloglog-uda.cc
M be/src/util/bit-util.h
M testdata/workloads/functional-query/queries/QueryTest/alter-table-set-column-stats.test
M testdata/workloads/functional-query/queries/QueryTest/compute-stats-incremental.test
M testdata/workloads/functional-query/queries/QueryTest/compute-stats.test
M testdata/workloads/functional-query/queries/QueryTest/show-stats.test
M testdata/workloads/functional-query/queries/QueryTest/truncate-table.test
8 files changed, 43 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/5004/1
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4433: Fix undefined NDV calculations
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4433: Fix undefined NDV calculations
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5004/1//COMMIT_MSG
Commit Message:
Line 10: argument is 0. This patch handles that 0 case, which can depend on the
> Might be good to mention that varies depending on the architecture.
Done
http://gerrit.cloudera.org:8080/#/c/5004/1/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:
Line 1175: // Use the lower bits to index into the number of streams and then find the first 1 bit
> Same comment as the other place - I think we can remove this branch.
Done
http://gerrit.cloudera.org:8080/#/c/5004/1/be/src/udf_samples/hyperloglog-uda.cc
File be/src/udf_samples/hyperloglog-uda.cc:
Line 75: // Use the lower bits to index into the number of streams and then find the first 1 bit
> Let's remove this branch while we're at it. It doesn't make sense to me why
Done
PS1, Line 81: (sizeof(hash_value) * CHAR_BIT - HLL_PRECISION));
: dst->ptr[idx] = ::max(dst->ptr[idx], first_one_bit);
> Can't we use the builtin you
It's in Impala proper, while it seems like the non-test files in this directory carefully avoid using anything from Impala proper.
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Fix undefined calls to builtin ctz.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: Fix undefined calls to __builtin_ctz.
......................................................................
Patch Set 5:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/62/
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4433: Fix undefined NDV calculations
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4433: Fix undefined NDV calculations
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5004/2/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 245:
missing function comments
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Fix undefined calls to builtin ctz.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: Fix undefined calls to __builtin_ctz.
......................................................................
Fix undefined calls to __builtin_ctz.
GCC's __builtin_ctz[l[l]] functions return undefined results when the
argument is 0. This patch handles that 0 case, which could otherwise
produce results that depend on the architecture.
Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Reviewed-on: http://gerrit.cloudera.org:8080/5004
Reviewed-by: Jim Apple <jb...@apache.org>
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exprs/aggregate-functions-ir.cc
M be/src/udf_samples/hyperloglog-uda.cc
M be/src/util/bit-util.h
3 files changed, 45 insertions(+), 21 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Jim Apple: Looks good to me, but someone else must approve
Dan Hecht: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] Fix undefined calls to builtin ctz.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: Fix undefined calls to __builtin_ctz.
......................................................................
Patch Set 5: Code-Review+1
rebase carry tim's +1
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4433: Fix undefined NDV calculations
Posted by "Amos Bird (Code Review)" <ge...@cloudera.org>.
Amos Bird has posted comments on this change.
Change subject: IMPALA-4433: Fix undefined NDV calculations
......................................................................
Patch Set 2:
Tested this patch on my Ubuntu server. The stats mismatching cases are gone. Thanks Jim ;)
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] Fix undefined calls to builtin ctz.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: Fix undefined calls to __builtin_ctz.
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] Fix undefined cals to builtin ctz.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: Fix undefined cals to __builtin_ctz.
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5004/3/be/src/udf_samples/hyperloglog-uda.cc
File be/src/udf_samples/hyperloglog-uda.cc:
PS3, Line 78: auto
uint64_t? Seems clearer to specify the type and it's not too verbose.
PS3, Line 80: hash_top_bits
hash_top_bits != 0 would make the intent clearer (it's a matter of taste, but that's generally what we do in the codebase).
http://gerrit.cloudera.org:8080/#/c/5004/3/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 32: #include <boost/type_traits/make_unsigned.hpp>
Is this used?
Line 247: unsigned int v, int otherwise = sizeof(unsigned int) * CHAR_BIT) {
We don't use the optional argument, so it seems like we should just remove that generality.
Would be good to explain that __builtin_ctz(0) is undefined too.
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Fix undefined calls to builtin ctz.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: Fix undefined calls to __builtin_ctz.
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] Fix undefined calls to builtin ctz.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: Fix undefined calls to __builtin_ctz.
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] Fix undefined cals to builtin ctz.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5004
to look at the new patch set (#3).
Change subject: Fix undefined cals to __builtin_ctz.
......................................................................
Fix undefined cals to __builtin_ctz.
GCC's __builtin_ctz[l[l]] functions return undefined results when the
argument is 0. This patch handles that 0 case, which could otherwise
produce results that depend on the architecture.
Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
---
M be/src/exprs/aggregate-functions-ir.cc
M be/src/udf_samples/hyperloglog-uda.cc
M be/src/util/bit-util.h
3 files changed, 43 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/5004/3
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4433: Fix undefined NDV calculations
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#2).
Change subject: IMPALA-4433: Fix undefined NDV calculations
......................................................................
IMPALA-4433: Fix undefined NDV calculations
GCC's __builtin_ctz[l[l]] functions return undefined results when the
argument is 0. This patch handles that 0 case, which can depend on the
architecture.
This changes the metadata for the test data, because ctz is used in
HLL++, which is used as the NDV computation in compute stats.
Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
---
M be/src/exprs/aggregate-functions-ir.cc
M be/src/udf_samples/hyperloglog-uda.cc
M be/src/util/bit-util.h
M testdata/workloads/functional-query/queries/QueryTest/alter-table-set-column-stats.test
M testdata/workloads/functional-query/queries/QueryTest/compute-stats-incremental.test
M testdata/workloads/functional-query/queries/QueryTest/compute-stats.test
M testdata/workloads/functional-query/queries/QueryTest/show-stats.test
M testdata/workloads/functional-query/queries/QueryTest/truncate-table.test
8 files changed, 51 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/5004/2
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4433: Fix undefined NDV calculations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4433: Fix undefined NDV calculations
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
Would be good to have someone else take a look in case there might be other consequences for tests.
http://gerrit.cloudera.org:8080/#/c/5004/1/be/src/udf_samples/hyperloglog-uda.cc
File be/src/udf_samples/hyperloglog-uda.cc:
PS1, Line 81: (sizeof(hash_value) * CHAR_BIT - HLL_PRECISION));
: dst->ptr[idx] = ::max(dst->ptr[idx], first_one_bit);
> It's in Impala proper, while it seems like the non-test files in this direc
Oops, thought I deleted this comment (I realised that point halfway through writing it)
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4433: Fix undefined NDV calculations
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has abandoned this change.
Change subject: IMPALA-4433: Fix undefined NDV calculations
......................................................................
Abandoned
Doesn't bring consensus to all the platforms on the NDV of timestamp_col. Let's bring the discussion back to IMPALA-4433 while we try and diagnose the actual platform-dependent code.
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4433: Fix undefined NDV calculations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4433: Fix undefined NDV calculations
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5004/1//COMMIT_MSG
Commit Message:
Line 10: argument is 0. This patch handles that 0 case.
Might be good to mention that varies depending on the architecture.
http://gerrit.cloudera.org:8080/#/c/5004/1/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:
Line 1175: if (hash_value != 0) {
Same comment as the other place - I think we can remove this branch.
http://gerrit.cloudera.org:8080/#/c/5004/1/be/src/udf_samples/hyperloglog-uda.cc
File be/src/udf_samples/hyperloglog-uda.cc:
Line 75: if (hash_value != 0) {
Let's remove this branch while we're at it. It doesn't make sense to me why it's there in the first place and I don't think it's consequential (if you think about it, getting a hash value of 0 should be extremely rare and not affect the final probabilistic result).
I wonder if the branch was originally an attempt to work around the problem you're fixing.
PS1, Line 81: hash_top_bits ? __builtin_ctzll(hash_top_bits) :
: (sizeof(hash_value) * CHAR_BIT - HLL_PRECISION)
Can't we use the builtin you
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Fix undefined calls to builtin ctz.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5004
to look at the new patch set (#4).
Change subject: Fix undefined calls to __builtin_ctz.
......................................................................
Fix undefined calls to __builtin_ctz.
GCC's __builtin_ctz[l[l]] functions return undefined results when the
argument is 0. This patch handles that 0 case, which could otherwise
produce results that depend on the architecture.
Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
---
M be/src/exprs/aggregate-functions-ir.cc
M be/src/udf_samples/hyperloglog-uda.cc
M be/src/util/bit-util.h
3 files changed, 45 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/5004/4
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4433: Fix undefined NDV calculations
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has restored this change.
Change subject: IMPALA-4433: Fix undefined NDV calculations
......................................................................
Restored
No longer needed for stats, but might as well eliminate undefined behavior
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: restore
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] Fix undefined calls to builtin ctz.
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: Fix undefined calls to __builtin_ctz.
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5004/3/be/src/udf_samples/hyperloglog-uda.cc
File be/src/udf_samples/hyperloglog-uda.cc:
PS3, Line 78: uint
> uint64_t? Seems clearer to specify the type and it's not too verbose.
Done
PS3, Line 80: (hash_top_bit
> hash_top_bits != 0 would make the intent clearer (it's a matter of taste, b
Done
http://gerrit.cloudera.org:8080/#/c/5004/3/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 32: #include <boost/type_traits/make_unsigned.hpp>
> Is this used?
Yes, but I was just moving it in the #include list, not using it. It's used elsewhere.
Line 247: /// zero.
> We don't use the optional argument, so it seems like we should just remove
We do use the optional argument in be/src/exprs/aggregate-functions-ir.cc.
Explained the undefinedness in a comment.
--
To view, visit http://gerrit.cloudera.org:8080/5004
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8460bc3f7e510ce07b8673387c9440accc432abe
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Amos Bird <am...@gmail.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes