Skip to content

Commit

Permalink
Back out "array_intersect operand swapping" (facebookincubator#10722)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: facebookincubator#10722

Original commit changeset: 0b5b0eb0a72d

Original Phabricator Diff: D60531033

This change causes non-deterministic behavior.

Reviewed By: xiaoxmeng

Differential Revision: D61163604
  • Loading branch information
Daniel Hunte authored and facebook-github-bot committed Aug 13, 2024
1 parent 99f990c commit 5cf7ffe
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 42 deletions.
33 changes: 2 additions & 31 deletions velox/functions/prestosql/ArrayIntersectExcept.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,6 @@ class ArrayIntersectExceptFunction : public exec::VectorFunction {
/// If the rhs values passed to either array_intersect() or array_except()
/// are constant (array literals) we create a set before instantiating the
/// object and pass as a constructor parameter (constantSet).
///
/// Smallest array optimization:
///
/// If the rhs values passed to array_intersect() are not constant we create
/// sets from whichever side has the smallest sum of lengths in the batch.

ArrayIntersectExceptFunction() = default;

Expand All @@ -133,30 +128,6 @@ class ArrayIntersectExceptFunction : public exec::VectorFunction {
BaseVector* right = args[1].get();

exec::LocalDecodedVector leftHolder(context, *left, rows);
exec::LocalDecodedVector rightHolder(context, *right, rows);

if (isIntersect && !constantSet_.has_value()) {
// Swap left and right if needed so that the right array has the smaller
// number of elements since the right will be made into a hash set.
vector_size_t leftSize = 0;
vector_size_t rightSize = 0;
const ArrayVector* leftArrayVector =
leftHolder.get()->base()->as<ArrayVector>();
const ArrayVector* rightArrayVector =
rightHolder.get()->base()->as<ArrayVector>();
rows.applyToSelected([&](vector_size_t row) {
vector_size_t leftidx = leftHolder.get()->index(row);
leftSize += leftArrayVector->sizeAt(leftidx);

vector_size_t rightidx = rightHolder.get()->index(row);
rightSize += rightArrayVector->sizeAt(rightidx);
});
if (leftSize < rightSize) {
std::swap(left, right);
std::swap(leftHolder, rightHolder);
}
}

auto decodedLeftArray = leftHolder.get();
auto baseLeftArray = decodedLeftArray->base()->as<ArrayVector>();

Expand Down Expand Up @@ -221,9 +192,9 @@ class ArrayIntersectExceptFunction : public exec::VectorFunction {
// (check outputSet).
bool addValue = false;
if constexpr (isIntersect) {
addValue = rightSet.set.contains(val);
addValue = rightSet.set.count(val) > 0;
} else {
addValue = !rightSet.set.contains(val);
addValue = rightSet.set.count(val) == 0;
}
if (addValue) {
auto it = outputSet.set.insert(val);
Expand Down
17 changes: 6 additions & 11 deletions velox/functions/prestosql/tests/ArrayIntersectTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -271,12 +271,7 @@ TEST_F(ArrayIntersectTest, constant) {
{},
{1, -2, 4},
});
// Test wiith right hand side being constant and larger than left hand side.
testExpr(
expected,
"array_intersect(C0, ARRAY[1,4,-2,10,11,12,13,14,15])",
{array1});

testExpr(expected, "array_intersect(C0, ARRAY[1,4,-2])", {array1});
testExpr(expected, "array_intersect(ARRAY[1,-2,4], C0)", {array1});
testExpr(
expected, "array_intersect(ARRAY[1,1,-2,1,-2,4,1,4,4], C0)", {array1});
Expand Down Expand Up @@ -313,10 +308,10 @@ TEST_F(ArrayIntersectTest, deterministic) {
testExpr(expectedC0C1, "array_intersect(C0, C1)", {c0, c1});

// C1 then C0.
// Since C1 has more elements, it should be swapped with C0 and
// the order of the result should be based on C1.
auto expectedC1C0 = expectedC0C1;

auto expectedC1C0 = makeNullableArrayVector<int32_t>({
{1, 4, -2},
{1, 4, -2},
});
testExpr(expectedC1C0, "array_intersect(C1, C0)", {c0, c1});
testExpr(expectedC1C0, "array_intersect(ARRAY[1,4,-2], C0)", {c0});
}
Expand All @@ -331,6 +326,6 @@ TEST_F(ArrayIntersectTest, dictionaryEncodedElementsInConstant) {
auto expected = makeArrayVector<int32_t>({{1, 3}, {2}, {}});
testExpr(
expected,
"array_intersect(testing_dictionary_array_elements(ARRAY [2, 2, 3, 1, 2, 2]), c0)",
"array_intersect(c0, testing_dictionary_array_elements(ARRAY [2, 2, 3, 1, 2, 2]))",
{array});
}

0 comments on commit 5cf7ffe

Please sign in to comment.