Skip to content

Commit 63fa93a

Browse files
mbasmanovafacebook-github-bot
authored andcommitted
Fix Array/Map/RowVector::copyRanges for UNKNOWN source (#6607)
Summary: Fix crashes when copying from TypeKind::UNKNOWN vector in the following methods: - Map/Array/RowVector::copyRanges - FlatVector<StringView>::copyRanges - FlatVector<bool>::copy(source, rows, toSourceRow) - RowVector::copy(source, rows, toSourceRow) Fixes #6606 Pull Request resolved: #6607 Reviewed By: xiaoxmeng Differential Revision: D49348224 Pulled By: mbasmanova fbshipit-source-id: 57598428a36787ee3c57a1b7c9f6cffd11206bf6
1 parent fe01c0c commit 63fa93a

File tree

5 files changed

+262
-45
lines changed

5 files changed

+262
-45
lines changed

velox/vector/ComplexVector.cpp

Lines changed: 72 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,11 @@ void RowVector::copy(
180180
const BaseVector* source,
181181
const SelectivityVector& rows,
182182
const vector_size_t* toSourceRow) {
183+
if (source->typeKind() == TypeKind::UNKNOWN) {
184+
rows.applyToSelected([&](auto row) { setNull(row, true); });
185+
return;
186+
}
187+
183188
// Copy non-null values.
184189
SelectivityVector nonNullRows = rows;
185190

@@ -261,13 +266,47 @@ void RowVector::copy(
261266
}
262267
}
263268

269+
namespace {
270+
271+
// Runs quick checks to determine whether input vector has only null values.
272+
// @return true if vector has only null values; false if vector may have
273+
// non-null values.
274+
bool isAllNullVector(const BaseVector& vector) {
275+
if (vector.typeKind() == TypeKind::UNKNOWN) {
276+
return true;
277+
}
278+
279+
if (vector.isConstantEncoding()) {
280+
return vector.isNullAt(0);
281+
}
282+
283+
auto leafVector = vector.wrappedVector();
284+
if (leafVector->isConstantEncoding()) {
285+
// A null constant does not have a value vector, so wrappedVector
286+
// returns the constant.
287+
VELOX_CHECK(leafVector->isNullAt(0));
288+
return true;
289+
}
290+
return false;
291+
}
292+
} // namespace
293+
264294
void RowVector::copyRanges(
265295
const BaseVector* source,
266296
const folly::Range<const CopyRange*>& ranges) {
267297
if (ranges.empty()) {
268298
return;
269299
}
270300

301+
if (isAllNullVector(*source)) {
302+
for (const auto& range : ranges) {
303+
for (auto i = 0; i < range.count; ++i) {
304+
setNull(range.targetIndex + i, true);
305+
}
306+
}
307+
return;
308+
}
309+
271310
auto minTargetIndex = std::numeric_limits<vector_size_t>::max();
272311
auto maxTargetIndex = std::numeric_limits<vector_size_t>::min();
273312
for (auto& r : ranges) {
@@ -422,23 +461,29 @@ void ArrayVectorBase::copyRangesImpl(
422461
const BaseVector* source,
423462
const folly::Range<const BaseVector::CopyRange*>& ranges,
424463
VectorPtr* targetValues,
425-
const BaseVector* sourceValues,
426-
VectorPtr* targetKeys,
427-
const BaseVector* sourceKeys) {
428-
auto sourceValue = source->wrappedVector();
429-
if (sourceValue->isConstantEncoding()) {
430-
// A null constant does not have a value vector, so wrappedVector
431-
// returns the constant.
432-
VELOX_CHECK(sourceValue->isNullAt(0));
433-
for (auto& r : ranges) {
434-
for (auto i = 0; i < r.count; ++i) {
435-
setNull(r.targetIndex + i, true);
464+
VectorPtr* targetKeys) {
465+
if (isAllNullVector(*source)) {
466+
for (const auto& range : ranges) {
467+
for (auto i = 0; i < range.count; ++i) {
468+
setNull(range.targetIndex + i, true);
436469
}
437470
}
438471
return;
439472
}
440-
VELOX_CHECK_EQ(sourceValue->encoding(), encoding());
441-
auto sourceArray = sourceValue->asUnchecked<ArrayVectorBase>();
473+
474+
const BaseVector* sourceValues;
475+
const BaseVector* sourceKeys = nullptr;
476+
477+
auto leafSource = source->wrappedVector();
478+
VELOX_CHECK_EQ(leafSource->encoding(), encoding());
479+
480+
if (typeKind_ == TypeKind::ARRAY) {
481+
sourceValues = leafSource->as<ArrayVector>()->elements().get();
482+
} else {
483+
sourceValues = leafSource->as<MapVector>()->mapValues().get();
484+
sourceKeys = leafSource->as<MapVector>()->mapKeys().get();
485+
}
486+
442487
if (targetKeys) {
443488
BaseVector::ensureWritable(
444489
SelectivityVector::empty(),
@@ -452,6 +497,8 @@ void ArrayVectorBase::copyRangesImpl(
452497
pool(),
453498
*targetValues);
454499
}
500+
501+
auto sourceArray = leafSource->asUnchecked<ArrayVectorBase>();
455502
auto setNotNulls = mayHaveNulls() || source->mayHaveNulls();
456503
auto* mutableOffsets = offsets_->asMutable<vector_size_t>();
457504
auto* mutableSizes = sizes_->asMutable<vector_size_t>();
@@ -869,6 +916,12 @@ void ArrayVector::validate(const VectorValidateOptions& options) const {
869916
elements_->validate(options);
870917
}
871918

919+
void ArrayVector::copyRanges(
920+
const BaseVector* source,
921+
const folly::Range<const CopyRange*>& ranges) {
922+
copyRangesImpl(source, ranges, &elements_, nullptr);
923+
}
924+
872925
bool MapVector::containsNullAt(vector_size_t idx) const {
873926
if (BaseVector::isNullAt(idx)) {
874927
return true;
@@ -1158,5 +1211,11 @@ void MapVector::validate(const VectorValidateOptions& options) const {
11581211
values_->validate(options);
11591212
}
11601213

1214+
void MapVector::copyRanges(
1215+
const BaseVector* source,
1216+
const folly::Range<const CopyRange*>& ranges) {
1217+
copyRangesImpl(source, ranges, &values_, &keys_);
1218+
}
1219+
11611220
} // namespace velox
11621221
} // namespace facebook

velox/vector/ComplexVector.h

Lines changed: 3 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -317,9 +317,7 @@ struct ArrayVectorBase : BaseVector {
317317
const BaseVector* source,
318318
const folly::Range<const CopyRange*>& ranges,
319319
VectorPtr* targetValues,
320-
const BaseVector* sourceValues,
321-
VectorPtr* targetKeys,
322-
const BaseVector* sourceKeys);
320+
VectorPtr* targetKeys);
323321

324322
void validateArrayVectorBase(
325323
const VectorValidateOptions& options,
@@ -407,20 +405,7 @@ class ArrayVector : public ArrayVectorBase {
407405

408406
void copyRanges(
409407
const BaseVector* source,
410-
const folly::Range<const CopyRange*>& ranges) override {
411-
const ArrayVector* sourceArray{};
412-
if (auto wrapped = source->wrappedVector();
413-
!wrapped->isConstantEncoding()) {
414-
sourceArray = wrapped->asUnchecked<ArrayVector>();
415-
}
416-
copyRangesImpl(
417-
source,
418-
ranges,
419-
&elements_,
420-
sourceArray ? sourceArray->elements_.get() : nullptr,
421-
nullptr,
422-
nullptr);
423-
}
408+
const folly::Range<const CopyRange*>& ranges) override;
424409

425410
uint64_t retainedSize() const override {
426411
return BaseVector::retainedSize() + offsets_->capacity() +
@@ -547,20 +532,7 @@ class MapVector : public ArrayVectorBase {
547532

548533
void copyRanges(
549534
const BaseVector* source,
550-
const folly::Range<const CopyRange*>& ranges) override {
551-
const MapVector* sourceMap{};
552-
if (auto wrapped = source->wrappedVector();
553-
!wrapped->isConstantEncoding()) {
554-
sourceMap = wrapped->asUnchecked<MapVector>();
555-
}
556-
copyRangesImpl(
557-
source,
558-
ranges,
559-
&values_,
560-
sourceMap ? sourceMap->values_.get() : nullptr,
561-
&keys_,
562-
sourceMap ? sourceMap->keys_.get() : nullptr);
563-
}
535+
const folly::Range<const CopyRange*>& ranges) override;
564536

565537
uint64_t retainedSize() const override {
566538
return BaseVector::retainedSize() + offsets_->capacity() +

velox/vector/FlatVector.cpp

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,11 @@ void FlatVector<bool>::copyValuesAndNulls(
5151
const BaseVector* source,
5252
const SelectivityVector& rows,
5353
const vector_size_t* toSourceRow) {
54+
if (source->typeKind() == TypeKind::UNKNOWN) {
55+
rows.applyToSelected([&](auto row) { setNull(row, true); });
56+
return;
57+
}
58+
5459
source = source->loadedVector();
5560
VELOX_CHECK(
5661
BaseVector::compatibleKind(BaseVector::typeKind(), source->typeKind()));
@@ -477,6 +482,15 @@ template <>
477482
void FlatVector<StringView>::copyRanges(
478483
const BaseVector* source,
479484
const folly::Range<const CopyRange*>& ranges) {
485+
if (source->typeKind() == TypeKind::UNKNOWN) {
486+
for (const auto& range : ranges) {
487+
for (auto i = 0; i < range.count; ++i) {
488+
setNull(range.targetIndex + i, true);
489+
}
490+
}
491+
return;
492+
}
493+
480494
auto leaf = source->wrappedVector()->asUnchecked<SimpleVector<StringView>>();
481495
if (pool_ == leaf->pool()) {
482496
// We copy referencing the storage of 'source'.

0 commit comments

Comments
 (0)