Skip to content

Commit

Permalink
Fix addNullsForUnselectedRows (facebookincubator#9279)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: facebookincubator#9279

addNullsForUnselectedRows used memcpy to copy bits from SelectivityVector into
the nulls buffer. Memcpy copies full bytes, hence, it copies a few extra bits
at the tail when rows.end() is not a multiple of 8. This causes lambda
functions that use addNullsForUnselectedRows to generate invalid vectors and
results in crashes.

See facebookincubator#5990 for additional context.

Reviewed By: bikramSingh91

Differential Revision: D55455300

fbshipit-source-id: e98b515b1f4f1876bacb7a483a305fba098522cd
  • Loading branch information
mbasmanova authored and facebook-github-bot committed Mar 28, 2024
1 parent a5ae9cc commit 0c86a0a
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 2 deletions.
10 changes: 8 additions & 2 deletions velox/functions/lib/LambdaFunctionUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,16 @@ BufferPtr addNullsForUnselectedRows(
const SelectivityVector& rows) {
// Set nulls for rows not present in 'rows'.
BufferPtr nulls = allocateNulls(vector->size(), vector->pool(), bits::kNull);
memcpy(

// bits::kNull is 0. Hence, bits::orBits() simply copies the bits from the
// selectivity vector into the nulls buffer. We cannot use memcpy because it
// will copy extra bits at the tail if rows.end() is not a multiple of 8.
bits::orBits(
nulls->asMutable<uint64_t>(),
rows.asRange().bits(),
bits::nbytes(rows.end()));
rows.begin(),
rows.end());

if (vector->nulls() != nullptr) {
// Transfer original nulls
bits::andBits(
Expand Down
1 change: 1 addition & 0 deletions velox/functions/lib/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ add_executable(
IsNullTest.cpp
IsNotNullTest.cpp
KllSketchTest.cpp
LambdaFunctionUtilTest.cpp
MapConcatTest.cpp
Re2FunctionsTest.cpp
RepeatTest.cpp
Expand Down
47 changes: 47 additions & 0 deletions velox/functions/lib/tests/LambdaFunctionUtilTest.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "velox/functions/lib/LambdaFunctionUtil.h"
#include "velox/vector/tests/utils/VectorTestBase.h"

namespace facebook::velox::functions {
namespace {

class LambdaFunctionUtilTest : public testing::Test,
public test::VectorTestBase {
protected:
static void SetUpTestCase() {
memory::MemoryManager::testingSetInstance({});
}
};

TEST_F(LambdaFunctionUtilTest, addNullsForUnselectedRows) {
auto vector = makeFlatVector<int64_t>({1, 2, 3, 4, 5});

SelectivityVector rows(3);
auto nulls = addNullsForUnselectedRows(vector, rows);

auto* rawNulls = nulls->as<uint64_t>();

EXPECT_FALSE(bits::isBitNull(rawNulls, 0));
EXPECT_FALSE(bits::isBitNull(rawNulls, 1));
EXPECT_FALSE(bits::isBitNull(rawNulls, 2));
EXPECT_TRUE(bits::isBitNull(rawNulls, 3));
EXPECT_TRUE(bits::isBitNull(rawNulls, 4));
}

} // namespace
} // namespace facebook::velox::functions

0 comments on commit 0c86a0a

Please sign in to comment.