You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@orc.apache.org by rip-nsk <gi...@git.apache.org> on 2018/01/19 23:05:10 UTC
[GitHub] orc pull request #212: ORC-293: [C++] Fix RleEncoderV1 for case when sizeof(...
GitHub user rip-nsk opened a pull request:
https://github.com/apache/orc/pull/212
ORC-293: [C++] Fix RleEncoderV1 for case when sizeof(long) < sizeof(int64_t)
and minor related changes.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rip-nsk/orc ORC-293
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/orc/pull/212.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #212
----
commit 3ec5b72cd4e44d475fdfdbdbf3f7c5089a174a6c
Author: rip-nsk <ri...@...>
Date: 2018-01-19T23:03:33Z
Fix RleEncoderV1 for case when sizeof(long) < sizeof(int64_t) and minor related changes.
----
---
[GitHub] orc pull request #212: ORC-293: [C++] Fix RleEncoderV1 for case when sizeof(...
Posted by xndai <gi...@git.apache.org>.
Github user xndai commented on a diff in the pull request:
https://github.com/apache/orc/pull/212#discussion_r162789950
--- Diff: c++/include/orc/Common.hh ---
@@ -69,7 +69,7 @@ namespace orc {
UNKNOWN_WRITER = INT32_MAX
};
- enum CompressionKind {
+ enum CompressionKind : std::int64_t {
--- End diff --
The definition of CompressionKind_MAX already makes CompressionKind a int64 type. Enum base type is a C++ 11 feature and should at least be configurable through orc-config.hh
---
[GitHub] orc issue #212: ORC-293: [C++] Fix RleEncoderV1 for case when sizeof(long) <...
Posted by xndai <gi...@git.apache.org>.
Github user xndai commented on the issue:
https://github.com/apache/orc/pull/212
+1
---
[GitHub] orc pull request #212: ORC-293: [C++] Fix RleEncoderV1 for case when sizeof(...
Posted by rip-nsk <gi...@git.apache.org>.
Github user rip-nsk commented on a diff in the pull request:
https://github.com/apache/orc/pull/212#discussion_r163724343
--- Diff: c++/include/orc/Common.hh ---
@@ -69,7 +69,7 @@ namespace orc {
UNKNOWN_WRITER = INT32_MAX
};
- enum CompressionKind {
+ enum CompressionKind : std::int64_t {
--- End diff --
I mean that constants CompressionKind_MAX and WriterVersion_MAX are not used and can be removed.
---
[GitHub] orc pull request #212: ORC-293: [C++] Fix RleEncoderV1 for case when sizeof(...
Posted by rip-nsk <gi...@git.apache.org>.
Github user rip-nsk commented on a diff in the pull request:
https://github.com/apache/orc/pull/212#discussion_r162755501
--- Diff: c++/src/RLEv1.cc ---
@@ -169,11 +169,11 @@ void RleEncoderV1::writeVslong(int64_t val) {
void RleEncoderV1::writeVulong(int64_t val) {
while (true) {
- if ((val & ~0x7f) == 0) {
+ if ((val & ~BASE_128_MASK) == 0) {
--- End diff --
It is added to match code in RleDecoderV1::readLong
---
[GitHub] orc pull request #212: ORC-293: [C++] Fix RleEncoderV1 for case when sizeof(...
Posted by rip-nsk <gi...@git.apache.org>.
Github user rip-nsk commented on a diff in the pull request:
https://github.com/apache/orc/pull/212#discussion_r162753893
--- Diff: c++/src/RLEv1.cc ---
@@ -148,7 +148,7 @@ void RleEncoderV1::write(int64_t value) {
numLiterals += 1;
} else {
numLiterals -= static_cast<int>(MINIMUM_REPEAT - 1);
- long base = literals[numLiterals];
+ int64_t base = literals[numLiterals];
--- End diff --
'auto' is more suitable, but it seems its usage is avoided here
---
[GitHub] orc pull request #212: ORC-293: [C++] Fix RleEncoderV1 for case when sizeof(...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/orc/pull/212
---
[GitHub] orc pull request #212: ORC-293: [C++] Fix RleEncoderV1 for case when sizeof(...
Posted by wgtmac <gi...@git.apache.org>.
Github user wgtmac commented on a diff in the pull request:
https://github.com/apache/orc/pull/212#discussion_r162755019
--- Diff: c++/src/RLEv1.cc ---
@@ -169,11 +169,11 @@ void RleEncoderV1::writeVslong(int64_t val) {
void RleEncoderV1::writeVulong(int64_t val) {
while (true) {
- if ((val & ~0x7f) == 0) {
+ if ((val & ~BASE_128_MASK) == 0) {
--- End diff --
I don't think we need change here as ~0x7f will be implicitly converted to int64_t.
Same for the below.
---
[GitHub] orc pull request #212: ORC-293: [C++] Fix RleEncoderV1 for case when sizeof(...
Posted by xndai <gi...@git.apache.org>.
Github user xndai commented on a diff in the pull request:
https://github.com/apache/orc/pull/212#discussion_r163463896
--- Diff: c++/include/orc/Common.hh ---
@@ -69,7 +69,7 @@ namespace orc {
UNKNOWN_WRITER = INT32_MAX
};
- enum CompressionKind {
+ enum CompressionKind : std::int64_t {
--- End diff --
We can potentially get rid of this enum and use proto::CompressionKind instead. It can be done in a separate change.
---
[GitHub] orc pull request #212: ORC-293: [C++] Fix RleEncoderV1 for case when sizeof(...
Posted by rip-nsk <gi...@git.apache.org>.
Github user rip-nsk commented on a diff in the pull request:
https://github.com/apache/orc/pull/212#discussion_r163337745
--- Diff: c++/include/orc/Common.hh ---
@@ -69,7 +69,7 @@ namespace orc {
UNKNOWN_WRITER = INT32_MAX
};
- enum CompressionKind {
+ enum CompressionKind : std::int64_t {
--- End diff --
What is the reason for these constants?
---