Skip to content

Commit

Permalink
Throw when null row is passed to multimap_from_entries (#11166)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #11166

Currently velox completely ignores null row entries in the input array whereas Presto throws when one is present. The change makes velox behave like Presto.

Reviewed By: kevinwilfong

Differential Revision: D63870933

fbshipit-source-id: 275500e375886eb8acbb72f57dc884c40590d4c0
  • Loading branch information
Daniel Hunte authored and facebook-github-bot committed Oct 8, 2024
1 parent e014dab commit 88888e7
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 3 deletions.
7 changes: 4 additions & 3 deletions velox/functions/prestosql/MultimapFromEntries.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,10 @@ struct MultimapFromEntriesFunction {
uniqueKeys_.clear();
uniqueKeys_.reserve(inputArray.size());

for (const auto& entry : inputArray.skipNulls()) {
const auto& key = entry.template at<0>();
const auto& value = entry.template at<1>();
for (const auto& entry : inputArray) {
VELOX_USER_CHECK(entry.has_value(), "map entry cannot be null");
const auto& key = entry.value().template at<0>();
const auto& value = entry.value().template at<1>();

VELOX_USER_CHECK(key.has_value(), "map key cannot be null");

Expand Down
24 changes: 24 additions & 0 deletions velox/functions/prestosql/tests/MultimapFromEntriesTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,5 +80,29 @@ TEST_F(MultimapFromEntriesTest, nullKey) {
"map key cannot be null");
}

TEST_F(MultimapFromEntriesTest, nullEntryFailure) {
RowVectorPtr rowVector = makeRowVector(
{makeFlatVector(std::vector<int64_t>{1, 2, 3}),
makeFlatVector(std::vector<int64_t>{4, 5, 6})});
rowVector->setNull(0, true);

ArrayVectorPtr input = makeArrayVector({0}, rowVector);

VELOX_ASSERT_THROW(
evaluate("multimap_from_entries(c0)", makeRowVector({input})),
"map entry cannot be null");

// Test that having a null entry in the base vector used to create the array
// vector does not cause a failure.
input = makeArrayVector({1}, rowVector);

auto expectedKeys = makeFlatVector<int64_t>({2, 3});
auto expectedValues = makeArrayVector<int64_t>({{5}, {6}});

auto expectedMap = makeMapVector({0}, expectedKeys, expectedValues);
auto result = evaluate("multimap_from_entries(c0)", makeRowVector({input}));
assertEqualVectors(expectedMap, result);
}

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

0 comments on commit 88888e7

Please sign in to comment.