Skip to content

Commit

Permalink
Modify map_top_n to throw when nested nulls are present (facebookincu…
Browse files Browse the repository at this point in the history
…bator#11157)

Summary:

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.

Differential Revision: D63796842
  • Loading branch information
Daniel Hunte authored and facebook-github-bot committed Oct 7, 2024
1 parent 4f11402 commit b2082dd
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})),
"Ordering nulls is not supported");
VELOX_ASSERT_THROW(
evaluate("map_top_n(c0, 10)", makeRowVector({data})),
"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 b2082dd

Please sign in to comment.