Skip to content

Prestissimo: Fix logic deserializing LongDecimal-typed Data Encoded in INT128_ARRAY in bytestream read #18838

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mbasmanova merged 1 commit into from
Feb 3, 2023
Merged

Prestissimo: Fix logic deserializing LongDecimal-typed Data Encoded in INT128_ARRAY in bytestream read #18838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions presto-native-execution/presto_cpp/presto_protocol/Base64Util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ struct ByteStream {

template <typename T>
T read() {
// Directly reading int128 values is not yet supported in ByteStream.
static_assert(sizeof(T) <= sizeof(uint64_t));
T value = *reinterpret_cast<const T*>(data_ + offset_);
offset_ += sizeof(T);
return value;
Expand All @@ -59,6 +61,16 @@ struct ByteStream {
int32_t offset_;
};

// ByteStream::read specialization for int128_t
template <>
velox::int128_t ByteStream::read<velox::int128_t>() {
// Fetching int128_t value by reading two 64-bit blocks rather than one
// 128-bit block to avoid general protection exception.
auto low = read<int64_t>();
auto high = read<int64_t>();
return velox::buildInt128(high, low);
}

velox::BufferPtr
readNulls(int32_t count, ByteStream& stream, velox::memory::MemoryPool* pool) {
bool mayHaveNulls = stream.read<bool>();
Expand Down
75 changes: 75 additions & 0 deletions presto-native-execution/presto_cpp/presto_protocol/tests/Base64Test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,81 @@ TEST_F(Base64Test, singleTinyint) {
ASSERT_EQ(1, intVector->valueAt(0));
}

TEST_F(Base64Test, simpleLongDecimal) {
// Note: string values of block representations in following test cases (e.g.,
// "data0") can be obtained by reading the actual block representations of
// corresponding unscaled values in running services of Prestissimo.

// Unscaled value = 0
const std::string data0 =
"DAAAAElOVDEyOF9BUlJBWQEAAAAAAAAAAAAAAAAAAAAAAAAAAA==";
Copy link
Copy Markdown
Collaborator

@majetideepak majetideepak Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These string values seem cryptic. Can we directly cast the unscaled values?

Copy link
Copy Markdown
Collaborator

@majetideepak majetideepak Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there is metadata involved. Can we construct the payload explicitly?

Copy link
Copy Markdown
Contributor

@karteekmurthys karteekmurthys Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markjin1990 Did you push changes to address this comment? It would be easier to verify what exact value been passed in the encoded string if you construct the payload.

Copy link
Copy Markdown
Collaborator

@majetideepak majetideepak Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markjin1990 how are these payloads even built now? Can you add a comment?

Copy link
Copy Markdown
Contributor Author

@markjin1990 markjin1990 Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markjin1990 how are these payloads even built now? Can you add a comment?

Just added. I basically synthesized some records of the corresponding decimal values in the table and print the value of the 2nd parameter of base64Encoded of readBlock to the screen in online running service of Prestissimo when reading these synthesized values. Hope that is clear.

Sorry that I haven't found an easy approach automatically synthesizing these values within the unit test and nor the other unit tests have demonstrated their approaches getting these values in the same test file.

auto vector0 = readBlock(LONG_DECIMAL(24, 2), data0, pool_.get());

ASSERT_EQ(TypeKind::LONG_DECIMAL, vector0->typeKind());
ASSERT_EQ(1, vector0->size());
ASSERT_FALSE(vector0->isNullAt(0));

auto decimalVector0 =
vector0->as<FlatVector<facebook::velox::UnscaledLongDecimal>>();
ASSERT_EQ(UnscaledLongDecimal(0), decimalVector0->valueAt(0));

// Unscaled value = 100
const std::string data1 =
"DAAAAElOVDEyOF9BUlJBWQEAAAAAZAAAAAAAAAAAAAAAAAAAAA==";
auto vector1 = readBlock(LONG_DECIMAL(24, 2), data1, pool_.get());

ASSERT_EQ(TypeKind::LONG_DECIMAL, vector1->typeKind());
ASSERT_EQ(1, vector1->size());
ASSERT_FALSE(vector1->isNullAt(0));

auto decimalVector1 =
vector1->as<FlatVector<facebook::velox::UnscaledLongDecimal>>();
ASSERT_EQ(UnscaledLongDecimal(100), decimalVector1->valueAt(0));

// Unscaled value = -100
const std::string data2 =
"DAAAAElOVDEyOF9BUlJBWQEAAAAAZAAAAAAAAAAAAAAAAAAAgA==";
auto vector2 = readBlock(LONG_DECIMAL(24, 2), data2, pool_.get());

ASSERT_EQ(TypeKind::LONG_DECIMAL, vector2->typeKind());
ASSERT_EQ(1, vector2->size());
ASSERT_FALSE(vector2->isNullAt(0));

auto decimalVector2 =
vector2->as<FlatVector<facebook::velox::UnscaledLongDecimal>>();
ASSERT_EQ(UnscaledLongDecimal(-100), decimalVector2->valueAt(0));
Copy link
Copy Markdown
Contributor

@karteekmurthys karteekmurthys Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add bigger values (> 64 bit) to the tests with positive and negative values.


// Unscaled value = 10^20
const std::string data3 =
"DAAAAElOVDEyOF9BUlJBWQEAAAAAAAAQYy1ex2sFAAAAAAAAAA==";
auto vector3 = readBlock(LONG_DECIMAL(24, 2), data3, pool_.get());

ASSERT_EQ(TypeKind::LONG_DECIMAL, vector3->typeKind());
ASSERT_EQ(1, vector3->size());
ASSERT_FALSE(vector3->isNullAt(0));

auto decimalVector3 =
vector3->as<FlatVector<facebook::velox::UnscaledLongDecimal>>();
ASSERT_EQ(
UnscaledLongDecimal(DecimalUtil::kPowersOfTen[20]),
decimalVector3->valueAt(0));

// Unscaled value = -10^20
const std::string data4 =
"DAAAAElOVDEyOF9BUlJBWQEAAAAAAAAQYy1ex2sFAAAAAAAAgA==";
auto vector4 = readBlock(LONG_DECIMAL(24, 2), data4, pool_.get());

ASSERT_EQ(TypeKind::LONG_DECIMAL, vector4->typeKind());
ASSERT_EQ(1, vector4->size());
ASSERT_FALSE(vector4->isNullAt(0));

auto decimalVector4 =
vector4->as<FlatVector<facebook::velox::UnscaledLongDecimal>>();
ASSERT_EQ(
UnscaledLongDecimal(-DecimalUtil::kPowersOfTen[20]),
decimalVector4->valueAt(0));
}

TEST_F(Base64Test, singleString) {
std::string data = "DgAAAFZBUklBQkxFX1dJRFRIAQAAAAoAAAAACgAAADIwMTktMTEtMTA=";

Expand Down