You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/04/20 12:46:21 UTC
[GitHub] [arrow] pitrou opened a new pull request #6991: ARROW-8529: [C++] Fix usage of NextCounts() on dictionary-encoded data
pitrou opened a new pull request #6991:
URL: https://github.com/apache/arrow/pull/6991
NextCounts() should be parametered with the dictionary index type, not the value type.
Previous code seems to have succeeded by chance on little-endian platforms.
See discussion in ARROW-8486.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #6991: ARROW-8529: [C++] Fix usage of NextCounts() on dictionary-encoded data
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #6991:
URL: https://github.com/apache/arrow/pull/6991#discussion_r411882849
##########
File path: cpp/src/arrow/util/rle_encoding.h
##########
@@ -414,6 +414,8 @@ static inline bool IndexInRange(int32_t idx, int32_t dictionary_length) {
template <typename T>
inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_length,
T* values, int batch_size) {
+ using IndexType = int32_t;
Review comment:
I think I misunderstood the issue for the unit testing. I guess this would have been caught if we had a big endian machine?
I think adding the comment with the link that @kiszk provided would still make sense for the casual reader?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #6991: ARROW-8529: [C++] Fix usage of NextCounts() on dictionary-encoded data
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #6991:
URL: https://github.com/apache/arrow/pull/6991#discussion_r411577540
##########
File path: cpp/src/arrow/util/rle_encoding.h
##########
@@ -414,6 +414,8 @@ static inline bool IndexInRange(int32_t idx, int32_t dictionary_length) {
template <typename T>
inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_length,
T* values, int batch_size) {
+ using IndexType = int32_t;
Review comment:
might be worth a comment here and below why IndexType is always static. Would it be possible to add a unit test with a larger value that would have failed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #6991: ARROW-8529: [C++] Fix usage of NextCounts() on dictionary-encoded data
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #6991:
URL: https://github.com/apache/arrow/pull/6991#discussion_r411416006
##########
File path: .github/workflows/cpp.yml
##########
@@ -228,7 +228,9 @@ jobs:
run: ci/scripts/util_checkout.sh
- name: Build
shell: bash
- run: ci/scripts/cpp_build.sh $(pwd) $(pwd)/build
+ run: |
+ export BOOST_ROOT=$BOOST_ROOT_1_72_0
+ ci/scripts/cpp_build.sh $(pwd) $(pwd)/build
Review comment:
This is an unrelated CI fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #6991: ARROW-8529: [C++] Fix usage of NextCounts() on dictionary-encoded data
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #6991:
URL: https://github.com/apache/arrow/pull/6991#discussion_r411578282
##########
File path: cpp/src/arrow/util/rle_encoding.h
##########
@@ -414,6 +414,8 @@ static inline bool IndexInRange(int32_t idx, int32_t dictionary_length) {
template <typename T>
inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_length,
T* values, int batch_size) {
+ using IndexType = int32_t;
Review comment:
You mean a larger type, or an index larger than 2**32?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #6991: ARROW-8529: [C++] Fix usage of NextCounts() on dictionary-encoded data
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #6991:
URL: https://github.com/apache/arrow/pull/6991#discussion_r411585778
##########
File path: cpp/src/arrow/util/rle_encoding.h
##########
@@ -414,6 +414,8 @@ static inline bool IndexInRange(int32_t idx, int32_t dictionary_length) {
template <typename T>
inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_length,
T* values, int batch_size) {
+ using IndexType = int32_t;
Review comment:
For Parquet use case, the max width of the index is 32 based on https://github.com/apache/parquet-format/blob/master/Encodings.md#dictionary-encoding-plain_dictionary--2-and-rle_dictionary--8
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on issue #6991: ARROW-8529: [C++] Fix usage of NextCounts() on dictionary-encoded data
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on issue #6991:
URL: https://github.com/apache/arrow/pull/6991#issuecomment-616529709
https://issues.apache.org/jira/browse/ARROW-8529
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #6991: ARROW-8529: [C++] Fix usage of NextCounts() on dictionary-encoded data
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #6991:
URL: https://github.com/apache/arrow/pull/6991#discussion_r412025224
##########
File path: cpp/src/arrow/util/rle_encoding.h
##########
@@ -414,6 +414,8 @@ static inline bool IndexInRange(int32_t idx, int32_t dictionary_length) {
template <typename T>
inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_length,
T* values, int batch_size) {
+ using IndexType = int32_t;
Review comment:
Note that this was previously simply `int`. The change here is simply to make things clearer and also note the index size explicitly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org