Skip to content

Commit

Permalink
Modify map_top_n to throw when nested nulls are present (#11157)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #11157

Nested nulls present inhibit the orderbility of values. Currently, any check for nested null is bypassed when 'n' is greater than the size of the map. This chanhe removes the bypass. This will now match Presto's behavior.

Reviewed By: kevinwilfong

Differential Revision: D63796842

fbshipit-source-id: 79ce7b15dea88ff6f7b81d17ed202195c0fdb5f9
  • Loading branch information
Daniel Hunte authored and facebook-github-bot committed Oct 8, 2024
1 parent 514740a commit e014dab
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 5 deletions.
5 changes: 0 additions & 5 deletions velox/functions/prestosql/MapTopN.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,6 @@ struct MapTopNFunction {
return;
}

if (n >= inputMap.size()) {
out.copy_from(inputMap);
return;
}

using It = typename arg_type<Map<Orderable<T1>, Orderable<T2>>>::Iterator;

Compare<It> comparator;
Expand Down
16 changes: 16 additions & 0 deletions velox/functions/prestosql/tests/MapTopNTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,22 @@ TEST_F(MapTopNTest, basic) {
"n must be greater than or equal to 0");
}

TEST_F(MapTopNTest, nestedNullFailure) {
auto data = makeMapVector(
/*offsets=*/{0},
/*keyVector=*/makeFlatVector<int32_t>({1, 2, 3}),
/*valueVector=*/
makeNullableArrayVector<int32_t>({{std::nullopt}, {2}, {5}}));

// Nested nulls present inhibit the orderbility of values. Expect an error.
VELOX_ASSERT_THROW(
evaluate("map_top_n(c0, 1)", makeRowVector({data})), // n < map size
"Ordering nulls is not supported");
VELOX_ASSERT_THROW(
evaluate("map_top_n(c0, 10)", makeRowVector({data})), // n > map size
"Ordering nulls is not supported");
}

// Test to ensure we use keys to break ties if values are
// equal.
TEST_F(MapTopNTest, equalValues) {
Expand Down

0 comments on commit e014dab

Please sign in to comment.