Modify map_top_n to throw when nested nulls are present #11157
Conversation
|
This pull request was exported from Phabricator. Differential Revision: D63796842 |
facebook-github-bot
added
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
…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
DanielHunte
force-pushed
the
export-D63796842
branch
from
3a5069b to
b2082dd
Compare
|
This pull request was exported from Phabricator. Differential Revision: D63796842 |
…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
DanielHunte
force-pushed
the
export-D63796842
branch
from
b2082dd to
289dbad
Compare
|
This pull request was exported from Phabricator. Differential Revision: D63796842 |
| @@ -60,11 +60,6 @@ struct MapTopNFunction { | |||
| return; | |||
| } | |||
|
|
|||
| if (n >= inputMap.size()) { | |||
| out.copy_from(inputMap); | |||
There was a problem hiding this comment.
The problem is we don't want to throw if the value itself is null, but only if it's a complex type that contains null. There's a way we could do this by accessing the key and element Vectors and iterating over them checking if they are null and if not, checking if they contain null. But assuming N is generally small the tradeoff between complexity and efficiency probably isn't worth it.
…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. Reviewed By: kevinwilfong Differential Revision: D63796842
DanielHunte
force-pushed
the
export-D63796842
branch
from
289dbad to
271f719
Compare
|
This pull request was exported from Phabricator. Differential Revision: D63796842 |
|
This pull request has been merged in e014dab. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
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