From 614458f3366231682d0fff2a474da08dddac72f8 Mon Sep 17 00:00:00 2001 From: Daniel Hunte Date: Mon, 7 Oct 2024 10:42:39 -0700 Subject: [PATCH] Throw when null row is passed to multimap_from_entries (#11166) Summary: 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 --- .../functions/prestosql/MultimapFromEntries.h | 7 +++--- .../tests/MultimapFromEntriesTest.cpp | 24 +++++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/velox/functions/prestosql/MultimapFromEntries.h b/velox/functions/prestosql/MultimapFromEntries.h index 05d86d0273d9..a9d448c51c1d 100644 --- a/velox/functions/prestosql/MultimapFromEntries.h +++ b/velox/functions/prestosql/MultimapFromEntries.h @@ -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"); diff --git a/velox/functions/prestosql/tests/MultimapFromEntriesTest.cpp b/velox/functions/prestosql/tests/MultimapFromEntriesTest.cpp index 1404c3fe5568..8c5aeed6884a 100644 --- a/velox/functions/prestosql/tests/MultimapFromEntriesTest.cpp +++ b/velox/functions/prestosql/tests/MultimapFromEntriesTest.cpp @@ -80,5 +80,29 @@ TEST_F(MultimapFromEntriesTest, nullKey) { "map key cannot be null"); } +TEST_F(MultimapFromEntriesTest, nullEntryFailure) { + RowVectorPtr rowVector = makeRowVector( + {makeFlatVector(std::vector{1, 2, 3}), + makeFlatVector(std::vector{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({2, 3}); + auto expectedValues = makeArrayVector({{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