Add support for Long Decimal Type in Native Parquet Reader #2822
Conversation
✅ Deploy Preview for meta-velox canceled.
|
facebook-github-bot
added
the
CLA Signed
velox/dwio/common/ColumnVisitors.h
Outdated
| template <typename A> | ||
| struct LoadIndices<int128_t, A> { | ||
| static xsimd::batch<int32_t, A> apply(const int128_t* values, const A& arch) { | ||
| VELOX_UNSUPPORTED("LoadIndices for int128_t type is not supported"); |
There was a problem hiding this comment.
How about just VELOX_UNREACHABLE()?
velox/dwio/common/IntDecoder.h
Outdated
| reinterpret_cast<char*>(&result) + (16 - numBytes), | ||
| reinterpret_cast<const char*>(valueOffset), | ||
| numBytes); | ||
| auto low = __builtin_bswap64(result >> 64); |
There was a problem hiding this comment.
Can we use __builtin_bswap128 here?
There was a problem hiding this comment.
On my mac, I see the following error. I guess this version is not supported everywhere.
error: use of undeclared identifier '__builtin_bswap128'
return __builtin_bswap128(result);
There was a problem hiding this comment.
Can we do this?
#if __has_builtin(__builtin_bswap128)
return __builtin_bswap128(result);
#else
return buildInt128(__builtin_bswap64(result), __builtin_bswap64(result >> 64));
#endif
velox/dwio/common/TypeUtil.h
Outdated
| @@ -53,6 +53,11 @@ struct make_index<uint64_t> { | |||
| using type = uint64_t; | |||
| }; | |||
|
|
|||
| template <> | |||
| struct make_index<int128_t> { | |||
| using type = int128_t; | |||
There was a problem hiding this comment.
uint128_t? Also if this is not really used, can we make sure it is not used at compile time?
There was a problem hiding this comment.
This is used. I fixed it.
velox/dwio/common/IntCodecCommon.h
Outdated
| @@ -43,4 +44,9 @@ constexpr int64_t MAX_NANOS = 999'999'999; | |||
| // 1 is reduced to epoch, as writer adds 1 for negative seconds. | |||
| constexpr int64_t MIN_SECONDS = INT64_MIN + (EPOCH_OFFSET - 1); | |||
|
|
|||
| #if not __has_builtin(__builtin_bswap128) | |||
| #define __builtin_bswap128(X) \ | |||
| buildInt128(__builtin_bswap64(X), __builtin_bswap64(X >> 64)) | |||
There was a problem hiding this comment.
Put extra parenthesis to avoid operator precedence issue: buildInt128(__builtin_bswap64((X)), __builtin_bswap64((X) >> 64))
Or just define an inline function, it's better this way because it also avoids duplicate evaluation if X is an expensive expression or has side-effect
majetideepak
force-pushed
the
add-long-decimal-support-native-parquet
branch
5 times, most recently
from
91f8af8
to
3905d3d
Compare
|
@Yuhta, all tests are passing. Can you take another look? Thanks. |
|
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
majetideepak
force-pushed
the
add-long-decimal-support-native-parquet
branch
from
3905d3d
to
0371d08
Compare
|
@majetideepak the |
|
@Yuhta fixed. I missed a change during rebase. Thanks! |
|
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…Velox memory pool instances for one Spark memory consumer (facebookincubator#2843) Closes facebookincubator#2822
No description provided.