Skip to content

Commit

Permalink
fix(vector): Add error logic to DictionaryVector's toString for loggi…
Browse files Browse the repository at this point in the history
…ng uninitialized lazy loaded vectors (#12025)

Summary:
Problem: There exists a blindspot in DictionaryVector's toString function, where lazy-loaded or otherwise unitialized vectors will be properly logged when calling toString, but may fail downstream elsewhere like during an expression eval because, although the logging suggests it was loaded, it was never properly initialized.

Solution: After discussing with Wei and Bikram, we should add logic to confirm the vector is loaded before and error out with a detailed error message before any string creating done. This will notify the user the vector is not-loaded and should be properly loaded before any logging is done.

Added additional test function to VectorTest class to cover this case.

(fixes #10594)

Differential Revision: D67870420
  • Loading branch information
peterenescu authored and facebook-github-bot committed Jan 10, 2025
1 parent ec2740f commit 4c4d106
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 0 deletions.
3 changes: 3 additions & 0 deletions velox/vector/DictionaryVector.h
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@ class DictionaryVector : public SimpleVector<T> {
}

std::string toString(vector_size_t index) const override {
VELOX_CHECK(
initialized_,
"Cannot convert to string because current DictionaryVector is not properly initialized yet.");
if (BaseVector::isNullAt(index)) {
return "null";
}
Expand Down
24 changes: 24 additions & 0 deletions velox/vector/tests/VectorTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3932,6 +3932,30 @@ TEST_F(VectorTest, testFlatteningOfRedundantDictionary) {
}
}

TEST_F(
VectorTest,
verifyOuterDictionaryLayersInitalizedWhenLoggingLazyLoadedVector) {
// Tester function verifies proper behavior when outer dictionary layers of a
// lazy vector are not properly initalized and user calls toString. Expected
// behavior should be a runtime error noting user to properly load vector
// prior to invoking toString.
auto doubleVector = makeFlatVector<double>({1.0, 2.0, 3.0});
auto doubleInDictionary = BaseVector::wrapInDictionary(
nullptr, makeIndices({0, 1, 2}), 3, doubleVector);
auto doubleInNestedDictionary = BaseVector::wrapInDictionary(
nullptr, makeIndices({0, 1, 2}), 3, doubleInDictionary);
auto doubleInLazyDictionary =
VectorFuzzer::wrapInLazyVector(doubleInNestedDictionary);

auto rowVector = makeRowVector({doubleVector, doubleInLazyDictionary});

// Log vector; in doing so, we should trigger our VELOX_CHECK error as
// dictionary is not properly initialized.
for (vector_size_t i = 0; i < rowVector->size(); ++i) {
EXPECT_THROW(rowVector->toString(i), VeloxRuntimeError);
}
}

TEST_F(VectorTest, hasOverlappingRanges) {
auto test = [this](
vector_size_t length,
Expand Down

0 comments on commit 4c4d106

Please sign in to comment.