Skip to content

Commit

Permalink
fix: Minor update to DictionaryVector toString (facebookincubator#12025)
Browse files Browse the repository at this point in the history
Summary:

Fixes error raised in Github issue 10594 (facebookincubator#10594)

Error details blindspot in toString function for lazy vectors where they are able to be logged; however they still show as unitialized as setInternalState was never executed.

I propose that function loadedVector be called to start the method as it will quit early for initialized vectors and serve virtually no additional overhead, while unloaded vectors will be properly initialized

Differential Revision: D67870420
  • Loading branch information
peterenescu authored and facebook-github-bot committed Jan 10, 2025
1 parent 923dcc8 commit 70a2569
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 70a2569

Please sign in to comment.