Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Array/Map/RowVector::copyRanges for UNKNOWN source #6607

Closed
wants to merge 1 commit into from
Closed

Fix Array/Map/RowVector::copyRanges for UNKNOWN source #6607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 72 additions & 13 deletions velox/vector/ComplexVector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,11 @@ void RowVector::copy(
const BaseVector* source,
const SelectivityVector& rows,
const vector_size_t* toSourceRow) {
if (source->typeKind() == TypeKind::UNKNOWN) {
rows.applyToSelected([&](auto row) { setNull(row, true); });
return;
}

// Copy non-null values.
SelectivityVector nonNullRows = rows;

Expand Down Expand Up @@ -261,13 +266,47 @@ void RowVector::copy(
}
}

namespace {

// Runs quick checks to determine whether input vector has only null values.
// @return true if vector has only null values; false if vector may have
// non-null values.
bool isAllNullVector(const BaseVector& vector) {
if (vector.typeKind() == TypeKind::UNKNOWN) {
return true;
}

if (vector.isConstantEncoding()) {
return vector.isNullAt(0);
}

auto leafVector = vector.wrappedVector();
if (leafVector->isConstantEncoding()) {
// A null constant does not have a value vector, so wrappedVector
// returns the constant.
VELOX_CHECK(leafVector->isNullAt(0));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: VELOX_USER_CHECK?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is not a user error, but rather a bug inside Velox as it should not allow creating vectors that would trigger this check.

return true;
}
return false;
}
} // namespace

void RowVector::copyRanges(
const BaseVector* source,
const folly::Range<const CopyRange*>& ranges) {
if (ranges.empty()) {
return;
}

if (isAllNullVector(*source)) {
for (const auto& range : ranges) {
for (auto i = 0; i < range.count; ++i) {
setNull(range.targetIndex + i, true);
}
}
return;
}

auto minTargetIndex = std::numeric_limits<vector_size_t>::max();
auto maxTargetIndex = std::numeric_limits<vector_size_t>::min();
for (auto& r : ranges) {
Expand Down Expand Up @@ -422,23 +461,29 @@ void ArrayVectorBase::copyRangesImpl(
const BaseVector* source,
const folly::Range<const BaseVector::CopyRange*>& ranges,
VectorPtr* targetValues,
const BaseVector* sourceValues,
VectorPtr* targetKeys,
const BaseVector* sourceKeys) {
auto sourceValue = source->wrappedVector();
if (sourceValue->isConstantEncoding()) {
// A null constant does not have a value vector, so wrappedVector
// returns the constant.
VELOX_CHECK(sourceValue->isNullAt(0));
for (auto& r : ranges) {
for (auto i = 0; i < r.count; ++i) {
setNull(r.targetIndex + i, true);
VectorPtr* targetKeys) {
if (isAllNullVector(*source)) {
for (const auto& range : ranges) {
for (auto i = 0; i < range.count; ++i) {
setNull(range.targetIndex + i, true);
}
}
return;
}
VELOX_CHECK_EQ(sourceValue->encoding(), encoding());
auto sourceArray = sourceValue->asUnchecked<ArrayVectorBase>();

const BaseVector* sourceValues;
const BaseVector* sourceKeys = nullptr;

auto leafSource = source->wrappedVector();
VELOX_CHECK_EQ(leafSource->encoding(), encoding());

if (typeKind_ == TypeKind::ARRAY) {
sourceValues = leafSource->as<ArrayVector>()->elements().get();
} else {
sourceValues = leafSource->as<MapVector>()->mapValues().get();
sourceKeys = leafSource->as<MapVector>()->mapKeys().get();
}

if (targetKeys) {
BaseVector::ensureWritable(
SelectivityVector::empty(),
Expand All @@ -452,6 +497,8 @@ void ArrayVectorBase::copyRangesImpl(
pool(),
*targetValues);
}

auto sourceArray = leafSource->asUnchecked<ArrayVectorBase>();
auto setNotNulls = mayHaveNulls() || source->mayHaveNulls();
auto* mutableOffsets = offsets_->asMutable<vector_size_t>();
auto* mutableSizes = sizes_->asMutable<vector_size_t>();
Expand Down Expand Up @@ -869,6 +916,12 @@ void ArrayVector::validate(const VectorValidateOptions& options) const {
elements_->validate(options);
}

void ArrayVector::copyRanges(
const BaseVector* source,
const folly::Range<const CopyRange*>& ranges) {
copyRangesImpl(source, ranges, &elements_, nullptr);
}

bool MapVector::containsNullAt(vector_size_t idx) const {
if (BaseVector::isNullAt(idx)) {
return true;
Expand Down Expand Up @@ -1158,5 +1211,11 @@ void MapVector::validate(const VectorValidateOptions& options) const {
values_->validate(options);
}

void MapVector::copyRanges(
const BaseVector* source,
const folly::Range<const CopyRange*>& ranges) {
copyRangesImpl(source, ranges, &values_, &keys_);
}

} // namespace velox
} // namespace facebook
34 changes: 3 additions & 31 deletions velox/vector/ComplexVector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,9 +317,7 @@ struct ArrayVectorBase : BaseVector {
const BaseVector* source,
const folly::Range<const CopyRange*>& ranges,
VectorPtr* targetValues,
const BaseVector* sourceValues,
VectorPtr* targetKeys,
const BaseVector* sourceKeys);
VectorPtr* targetKeys);

void validateArrayVectorBase(
const VectorValidateOptions& options,
Expand Down Expand Up @@ -407,20 +405,7 @@ class ArrayVector : public ArrayVectorBase {

void copyRanges(
const BaseVector* source,
const folly::Range<const CopyRange*>& ranges) override {
const ArrayVector* sourceArray{};
if (auto wrapped = source->wrappedVector();
!wrapped->isConstantEncoding()) {
sourceArray = wrapped->asUnchecked<ArrayVector>();
}
copyRangesImpl(
source,
ranges,
&elements_,
sourceArray ? sourceArray->elements_.get() : nullptr,
nullptr,
nullptr);
}
const folly::Range<const CopyRange*>& ranges) override;

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

void copyRanges(
const BaseVector* source,
const folly::Range<const CopyRange*>& ranges) override {
const MapVector* sourceMap{};
if (auto wrapped = source->wrappedVector();
!wrapped->isConstantEncoding()) {
sourceMap = wrapped->asUnchecked<MapVector>();
}
copyRangesImpl(
source,
ranges,
&values_,
sourceMap ? sourceMap->values_.get() : nullptr,
&keys_,
sourceMap ? sourceMap->keys_.get() : nullptr);
}
const folly::Range<const CopyRange*>& ranges) override;

uint64_t retainedSize() const override {
return BaseVector::retainedSize() + offsets_->capacity() +
Expand Down
14 changes: 14 additions & 0 deletions velox/vector/FlatVector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ void FlatVector<bool>::copyValuesAndNulls(
const BaseVector* source,
const SelectivityVector& rows,
const vector_size_t* toSourceRow) {
if (source->typeKind() == TypeKind::UNKNOWN) {
rows.applyToSelected([&](auto row) { setNull(row, true); });
return;
}

source = source->loadedVector();
VELOX_CHECK(
BaseVector::compatibleKind(BaseVector::typeKind(), source->typeKind()));
Expand Down Expand Up @@ -477,6 +482,15 @@ template <>
void FlatVector<StringView>::copyRanges(
const BaseVector* source,
const folly::Range<const CopyRange*>& ranges) {
if (source->typeKind() == TypeKind::UNKNOWN) {
for (const auto& range : ranges) {
for (auto i = 0; i < range.count; ++i) {
setNull(range.targetIndex + i, true);
}
}
return;
}

auto leaf = source->wrappedVector()->asUnchecked<SimpleVector<StringView>>();
if (pool_ == leaf->pool()) {
// We copy referencing the storage of 'source'.
Expand Down
Loading