Skip to content

Commit

Permalink
serialization of timestamp in spilling (facebookincubator#6838)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: facebookincubator#6838

Fix serialization of timestamp in spilling. TimeStamp does not always fit in
one int64_t as nanos. More information about the issue here.
facebookincubator#6837.

This fixes several fuzzer issues such as
facebookincubator#6833

Reviewed By: mbasmanova

Differential Revision: D49796384

fbshipit-source-id: c1c45ebc0e3d159e1fc2db1cd315dae5d1957d31
  • Loading branch information
laithsakka authored and facebook-github-bot committed Oct 4, 2023
1 parent 4018ddf commit 0e9cf61
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 5 deletions.
9 changes: 7 additions & 2 deletions velox/exec/tests/SpillTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,19 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#include "velox/exec/Spill.h"

#include <gtest/gtest.h>
#include <algorithm>
#include <memory>

#include "velox/common/base/RuntimeMetrics.h"
#include "velox/common/base/tests/GTestUtils.h"
#include "velox/common/file/FileSystems.h"
#include "velox/exec/OperatorUtils.h"
#include "velox/exec/Spill.h"
#include "velox/exec/tests/utils/TempDirectoryPath.h"
#include "velox/serializers/PrestoSerializer.h"
#include "velox/type/Timestamp.h"
#include "velox/vector/tests/utils/VectorTestBase.h"

using namespace facebook::velox;
Expand Down Expand Up @@ -424,7 +427,9 @@ TEST_P(SpillTest, spillTimestamp) {
Timestamp{12, 0},
Timestamp{0, 17'123'456},
Timestamp{1, 17'123'456},
Timestamp{-1, 17'123'456}};
Timestamp{-1, 17'123'456},
Timestamp{Timestamp::kMaxSeconds, Timestamp::kMaxNanos},
Timestamp{Timestamp::kMinSeconds, 0}};

SpillState state(
spillPath,
Expand Down
8 changes: 5 additions & 3 deletions velox/serializers/PrestoSerializer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,9 @@ void readValues<Timestamp>(
}

Timestamp readLosslessTimestamp(ByteStream* source) {
int64_t nanos = source->read<int64_t>();
return Timestamp::fromNanos(nanos);
int64_t seconds = source->read<int64_t>();
uint64_t nanos = source->read<uint64_t>();
return Timestamp(seconds, nanos);
}

void readLosslessTimestampValues(
Expand Down Expand Up @@ -1014,7 +1015,8 @@ template <>
void VectorStream::append(folly::Range<const Timestamp*> values) {
if (useLosslessTimestamp_) {
for (auto& value : values) {
appendOne(value.toNanos());
appendOne(value.getSeconds());
appendOne(value.getNanos());
}
} else {
for (auto& value : values) {
Expand Down

0 comments on commit 0e9cf61

Please sign in to comment.