Skip to content

Commit

Permalink
Seek to past the end is allowed
Browse files Browse the repository at this point in the history
Summary: Seek to the past-the-end element of a stripe is allowed.

Differential Revision: D56740881
  • Loading branch information
Daniel Munoz authored and facebook-github-bot committed Apr 30, 2024
1 parent abcd447 commit fa64d30
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 7 deletions.
2 changes: 1 addition & 1 deletion velox/dwio/common/OnDemandUnitLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class OnDemandUnitLoader : public UnitLoader {

void onSeek(uint32_t unit, uint64_t rowOffsetInUnit) override {
VELOX_CHECK_LT(unit, loadUnits_.size(), "Unit out of range");
VELOX_CHECK_LT(
VELOX_CHECK_LE(
rowOffsetInUnit, loadUnits_[unit]->getNumRows(), "Row out of range");
}

Expand Down
26 changes: 22 additions & 4 deletions velox/dwio/common/tests/OnDemandUnitLoaderTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -121,19 +121,37 @@ TEST(OnDemandUnitLoaderTests, CanSeek) {
EXPECT_EQ(blockedOnIoCount, 3);
}

TEST(OnDemandUnitLoaderTests, SeekOutOfRange) {
TEST(OnDemandUnitLoaderTests, SeekOutOfRangeReaderError) {
size_t blockedOnIoCount = 0;
OnDemandUnitLoaderFactory factory([&](auto) { ++blockedOnIoCount; });
ReaderMock readerMock{{10, 20, 30}, {0, 0, 0}, factory};
EXPECT_EQ(readerMock.unitsLoaded(), std::vector<bool>({false, false, false}));
EXPECT_EQ(blockedOnIoCount, 0);

readerMock.seek(60);

EXPECT_THAT(
[&]() { readerMock.seek(61); },
Throws<facebook::velox::VeloxRuntimeError>(Property(
&facebook::velox::VeloxRuntimeError::message,
HasSubstr("Can't seek to possition 61 in file. Must be up to 60."))));
}

TEST(OnDemandUnitLoaderTests, SeekOutOfRange) {
OnDemandUnitLoaderFactory factory(nullptr);
std::vector<std::atomic_bool> unitsLoaded(getUnitsLoadedWithFalse(1));
std::vector<std::unique_ptr<LoadUnit>> units;
units.push_back(std::make_unique<LoadUnitMock>(10, 0, unitsLoaded, 0));

auto unitLoader = factory.create(std::move(units));

unitLoader->onSeek(0, 10);

EXPECT_THAT(
[&]() { readerMock.seek(60); },
[&]() { unitLoader->onSeek(0, 11); },
Throws<facebook::velox::VeloxRuntimeError>(Property(
&facebook::velox::VeloxRuntimeError::message,
HasSubstr(
"Can't seek to possition 60 in file. Must be less than: 60."))));
HasSubstr("Row out of range"))));
}

TEST(OnDemandUnitLoaderTests, UnitOutOfRange) {
Expand Down
6 changes: 4 additions & 2 deletions velox/dwio/common/tests/utils/UnitLoaderTestTools.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,10 @@ void ReaderMock::seek(uint64_t rowNumber) {
rowsLeft -= rowCount;
totalRows += rowCount;
}
VELOX_FAIL(
"Can't seek to possition {} in file. Must be less than: {}.",
VELOX_CHECK_EQ(
rowsLeft,
0,
"Can't seek to possition {} in file. Must be up to {}.",
rowNumber,
totalRows);
}
Expand Down

0 comments on commit fa64d30

Please sign in to comment.