Add map_from_entries Presto function #3417

darrenfu · 2022-12-05T09:54:42Z

Summary

Adding the map_from_entries Presto function to Velox

map_from_entries(array(row(K, V))) -> map(K, V)

Returns a map created from the given array of entries.

For example:

SELECT map_from_entries(ARRAY[(1, 'x'), (2, 'y')]); -- {1 -> 'x', 2 -> 'y'}
SELECT map_from_entries(ARRAY[(1, 'x'), (2, null)]); -- {1 -> 'x', 2 -> null}
SELECT map_from_entries(ARRAY[(1, 'x'), (1, 'y')]); -- duplicate key error
SELECT map_from_entries(ARRAY[(null, 'x'), (1, 'y')]); -- map key cannot be null error
SELECT map_from_entries(ARRAY[cast(null as ROW(int, varchar)), (1, 'y')]); -- map entry cannot be null error

netlify · 2022-12-05T09:54:47Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`60e3240`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6486c382a05c920008c5fa2b

mbasmanova · 2022-12-12T19:54:21Z

@darrenfu CI is failing. Would you take a look?

darrenfu · 2022-12-13T23:31:36Z

@darrenfu CI is failing. Would you take a look?

The CI error is a discrepancy between the two eval and evalSimplify paths:
The latter will remove all the initial nulls (see #156), thus it will not throw an VeloxUserException if there is null in the input array. I believe this is unexpected because udf map_from_entries does not take a null map entry.

@pedroerp - since you are the author of #156, any advice to satisfy the fuzzer on both paths (while still throw a VeloxUserException for any nulls in the input array)?

mbasmanova

@darrenfu I think this function needs to be implemented as a vector function. See comment below for details. Also, to test this new function more thoroughly, please, run the fuzzer. First, test function in isolation, then in combination with other functions.

velox_expression_fuzzer_test --logtostderr=1 --velox_fuzzer_enable_complex_types --only map_from_entries --duration_sec 60

velox_expression_fuzzer_test --logtostderr=1 --velox_fuzzer_enable_complex_types --enable_variadic_signatures --duration_sec 600

velox/docs/functions/map.rst

velox/functions/prestosql/tests/MapFromEntriesTest.cpp

mbasmanova · 2022-12-15T10:20:57Z

@darrenfu CI is red. Would you take a look? Make sure to rebase onto latest main to pick up all the recent fixes.

mbasmanova

@darrenfu Some comments about the implementation. I expect the fuzzer test to fail for this function because it doesn't handle encodings properly. Curious to know if it does.

velox/functions/prestosql/MapFromEntries.cpp

darrenfu · 2022-12-16T08:59:53Z

For the CI failure, it relates to a fuzzer signature empty check. I will check with Wei.

I will also add more unit tests to cover more encodings and unknown (empty array) input.

mbasmanova

@darrenfu Darren, thank you for iterating on this PR. Working with complex types and encodings is indeed a bit tricky. Here are some suggestions.

mbasmanova · 2022-12-16T14:13:44Z

velox/functions/prestosql/MapFromEntries.cpp

+      localResult =
+          BaseVector::wrapInConstant(rows.size(), flatIndex, localResult);
+    } else {
+      exec::DecodedArgs decodedArgs(rows, args, context);


Here, the only possibility is that input is flat. Hence, no need to decode.

The easiest way to explore possible encodings is by using VectorFuzzer to generate random inputs.

Here is a rough sketch of what it may look like to handle all encodings.

https://github.com/facebookincubator/velox/compare/main...mbasmanova:velox-1:map_from_entries_simpleudf?expand=1

I modified the Fuzzer to workaround #3533 and also modified DecodedVector to workaround a bug that @kagamiori Wei is looking into (#3424)

velox_expression_fuzzer_test --enable_variadic_signatures --lazy_vector_generation_ratio=0.2 --velox_fuzzer_enable_complex_types --logtostderr=1 --only map_from_entries --duration_sec 60

Thank you!!! Your sketch is super helpful for me to decode such a nested complex vector, this comment really made my day!

FYI: I know the gist of different simple vector layouts when I was in Saber, but I am not super familiar with the surrounding helper / wrapper classes and the usage scenarios, like SelectivityVector, complex vector layouts (this is not contributed from Saber), DecodedVector (I wrote our own vector SerDe classes when I was in Saber). So excited to witness so many new velox features since then, and happy to refresh my arsenal.

Also thanks for your quick workaround for bug #3424 and #3533, I amended a comment - it actually affects a few map functions.

I modified the Fuzzer to workaround #3533 and also modified DecodedVector to workaround a bug that @kagamiori Wei is looking into (#3424)

Since Wei is working on the fuzzer bugs, I will leave the fuzzer code unchanged with CI failures. Once these two bugs are fixed, I will rebase from main and land this PR. Does it sound good?

Since Wei is working on the fuzzer bugs, I will leave the fuzzer code unchanged with CI failures. Once these two bugs are fixed, I will rebase from main and land this PR. Does it sound good?

Indeed, this PR has a dependency on fixing Fuzzer and DecodedVector. Just to clarify, after rebasing the PR, you'll need to re-run Fuzzer to make sure there are not failures and the PR will need to be reviewed before it is ready for landing.

@mbasmanova - regarding the fuzzer failure, I did some debugging and found that the exception is thrown at this line:

VectorPtr wrappedKeys; VectorPtr wrappedValues; if (decodedRow->isIdentityMapping()) { wrappedKeys = rowVector->childAt(0); wrappedValues = rowVector->childAt(1); } else { wrappedKeys = decodedRow->wrap( rowVector->childAt(0), *inputRowVector, inputRowVector->size()); // <---- THROW EXCEPTION: // "Dictionary index must be less than base vector's size. Index:..." // mostly reproduced at the simplified path + the duplicate-key case wrappedValues = decodedRow->wrap( rowVector->childAt(1), *inputRowVector, inputRowVector->size()); }

Question:
Why do you call decodedRow->wrap() to wrap the keys and values vectors for non-flat encodings in your sketch code? Maybe I'm wrong, but I feel we should compute the total size, new indices, and call BaseVector::wrapInDictionary() to wrap the keys and values. Refer to map()'s implementation. What do you think?

I noticed that this edge case usually happens when keys is a nested dictionary encoding or even more complex encoding where unit test cases don't cover.

Found the root cause, see #3687

velox/functions/prestosql/MapFromEntries.cpp

darrenfu · 2022-12-21T02:39:02Z

The current CI failure is suspicious to fuzzer's lack of support on nested complex types, aka. ARRAY(ROW<>).
See more discussion: #3388 (comment)

cc @bikramSingh91 for awareness.

bikramSingh91 · 2022-12-21T02:51:46Z

The current CI failure is suspicious to fuzzer's lack of support on nested complex types, aka. ARRAY(ROW<>). See more discussion: #3388 (comment)

cc @bikramSingh91 for awareness.

@darrenfu #3540 recently landed which now also picks return types for top level expressions from functions that have templated return types. therefore, it is now possible to pick zip with return type array(row(T,K,..)) and randomly bind the templated types to concrete types.

kagamiori · 2022-12-21T03:03:08Z

The current CI failure is suspicious to fuzzer's lack of support on nested complex types, aka. ARRAY(ROW<>). See more discussion: #3388 (comment)
cc @bikramSingh91 for awareness.

@darrenfu #3540 recently landed which now also picks return types for top level expressions from functions that have templated return types. therefore, it is now possible to pick zip with return type array(row(T,K,..)) and randomly bind the templated types to concrete types.

Sorry I haven't landed #3540 yet. It was blocked by #3564. I expect to land #3540 soon.

bikramSingh91 · 2022-12-21T03:05:03Z

The current CI failure is suspicious to fuzzer's lack of support on nested complex types, aka. ARRAY(ROW<>). See more discussion: #3388 (comment)
cc @bikramSingh91 for awareness.

@darrenfu #3540 recently landed which now also picks return types for top level expressions from functions that have templated return types. therefore, it is now possible to pick zip with return type array(row(T,K,..)) and randomly bind the templated types to concrete types.

Sorry I haven't landed #3540 yet. It was blocked by #3564. I expect to land #3540 soon.

Ah, Interesting, let me dive into this tomorrow and try to figure out what is happening then.

velox/functions/prestosql/MapFromEntries.cpp

mbasmanova · 2023-01-04T02:52:20Z

@darrenfu CircleCI is failing and the failure seems related to the new function. Would you take a look?

bikramSingh91 · 2023-01-04T03:26:48Z

@darrenfu CircleCI is failing and the failure seems related to the new function. Would you take a look?

I've detailed the root cause and fix of the issue here, please take a look.
#3581 (comment)

mbasmanova

@darrenfu Darren, thank you for iterating on the PR.

The function implementation looks good to me. Some comments about the tests.

velox/functions/prestosql/tests/MapFromEntriesTest.cpp

velox/docs/functions/json.rst

mbasmanova · 2023-01-10T16:42:17Z

@darrenfu Darren, CI is failing on this PR. Would you take a look?

darrenfu · 2023-05-26T17:27:29Z

cc @kgpai for the benchmark result upload failure (503 error). I hope it's a transient error.

kgpai · 2023-05-26T17:35:24Z

@darrenfu It was transient, though we are still working on that job and need to update the keys that would allow us to upload the results.

darrenfu · 2023-05-26T18:05:20Z

Hi Darren, Thank you for adding this function! It looks good to me overall. I left a few comments. Please also run fuzzer for 30 minutes to ensure no failures.

Thanks for mentioning that test approach. Yes, I just did locally. Nothing interesting is thrown.

Including the following changes: 1. use LocalSelectivityVector 2. add array and row encoding support 3. update map.rst with lex order 4. fix CI fuzzer failure by checking element.has_value() 5. fix issue facebookincubator#3581 6. fix CI fuzzer failure by set rows.end() as MapVector's length with unit test covered

darrenfu · 2023-06-12T18:45:28Z

@darrenfu Darren, thank you for iterating on the PR.

The function implementation looks good to me. Some comments about the tests.

Addressed all the comments for the unit tests and fuzz test. I believe the PR is in a ready-to-merge state. Let me know if there is anything else to address.

facebook-github-bot · 2023-06-12T21:10:47Z

@pranjalssh has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng · 2023-06-13T16:52:18Z

velox/functions/prestosql/MapFromEntries.cpp

+      const auto& flatArray = constantArray->valueVector();
+      const auto flatIndex = constantArray->index();
+
+      exec::LocalSelectivityVector singleRow(context, flatIndex + 1);


nit: s/singleRow/singleRowSelector/

xiaoxmeng · 2023-06-13T16:59:30Z

velox/functions/prestosql/MapFromEntries.cpp

+      const ArrayVector* inputArray,
+      const TypePtr& outputType,
+      exec::EvalCtx& context) const {
+    auto& inputRowVector = inputArray->elements();


nit: s/inputRowVector/inputValueVector/

xiaoxmeng · 2023-06-13T16:59:49Z

velox/functions/prestosql/MapFromEntries.cpp

+      const TypePtr& outputType,
+      exec::EvalCtx& context) const {
+    auto& inputRowVector = inputArray->elements();
+    exec::LocalDecodedVector decodedRow(context);


s/decodedRow/decodedValueVector/

xiaoxmeng · 2023-06-13T17:00:42Z

velox/functions/prestosql/MapFromEntries.cpp

+    auto& inputRowVector = inputArray->elements();
+    exec::LocalDecodedVector decodedRow(context);
+    decodedRow.get()->decode(*inputRowVector);
+    auto rowVector = decodedRow->base()->as<RowVector>();


s/rowVector/valueRowVector/

xiaoxmeng · 2023-06-13T17:00:56Z

velox/functions/prestosql/MapFromEntries.cpp

+    exec::LocalDecodedVector decodedRow(context);
+    decodedRow.get()->decode(*inputRowVector);
+    auto rowVector = decodedRow->base()->as<RowVector>();
+    auto rowKeyVector = rowVector->childAt(0);


s/rowKeyVector/keyValueVector/

xiaoxmeng · 2023-06-13T17:04:41Z

velox/functions/prestosql/MapFromEntries.cpp

+      context.applyToSelectedNoThrow(rows, [&](vector_size_t row) {
+        auto size = inputArray->sizeAt(row);
+        auto offset = inputArray->offsetAt(row);
+        for (auto i = 0; i < size; ++i) {


Can we simply check if size is zero or not?

Can we do that by just checking size == 0? The purpose is to validate all map entries and map keys are not null here.

xiaoxmeng · 2023-06-13T17:08:46Z

velox/functions/prestosql/MapFromEntries.cpp

+        auto size = inputArray->sizeAt(row);
+        auto offset = inputArray->offsetAt(row);
+        for (auto i = 0; i < size; ++i) {
+          bool isMapEntryNull = decodedRow->isNullAt(offset + i);


const bool isMapEntryNull?
dittos?

xiaoxmeng · 2023-06-13T17:21:36Z

velox/functions/prestosql/MapFromEntries.cpp

+        for (auto i = 0; i < size; ++i) {
+          bool isMapEntryNull = decodedRow->isNullAt(offset + i);
+          VELOX_USER_CHECK(!isMapEntryNull, "map entry cannot be null");
+          bool isMapKeyNull =


nit: const bool isMapKeyNull =

xiaoxmeng · 2023-06-13T17:21:47Z

velox/functions/prestosql/MapFromEntries.cpp

+        auto size = inputArray->sizeAt(row);
+        auto offset = inputArray->offsetAt(row);
+        for (auto i = 0; i < size; ++i) {
+          bool isMapEntryNull = decodedRow->isNullAt(offset + i);


nit: const bool isMapEntryNull

xiaoxmeng · 2023-06-13T17:31:18Z

velox/functions/prestosql/MapFromEntries.cpp

+      wrappedKeys = rowVector->childAt(0);
+      wrappedValues = rowVector->childAt(1);
+    } else {
+      wrappedKeys = decodedRow->wrap(


Where we handle the duplicate keys? Thanks!

It is handled via calling checkDuplicateKeys()

facebook-github-bot · 2023-06-13T17:50:02Z

@darrenfu merged this pull request in 0e6eb8f.

conbench-facebook · 2023-06-13T18:08:57Z

Conbench analyzed the 1 benchmark run on commit 0e6eb8fe.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

facebook-github-bot · 2023-06-13T19:42:16Z

This pull request has been reverted by 72ddf94.

Summary: In previous PR: facebookincubator#3417 (D46169043), the buck TARGETS file changes are overlooked. Thus it causes many internal build failures. This change is to amend those BUCK changes. Differential Revision: D46700832 fbshipit-source-id: 7eae818d3e320c2196968aa098e367ac633ca850

…or#5257) Summary: Pull Request resolved: facebookincubator#5257 In previous PR: facebookincubator#3417 (D46169043), the buck TARGETS file changes are overlooked. Thus it causes many internal build failures. This change is to amend those BUCK changes. Differential Revision: D46700832 fbshipit-source-id: afa9c6740e58837e900f6bf65947027f5b173b86

…or#5257) Summary: Pull Request resolved: facebookincubator#5257 In previous PR: facebookincubator#3417 (D46169043), the buck TARGETS file changes are overlooked. Thus it causes many internal build failures. This change is to amend those BUCK changes. Reviewed By: xiaoxmeng Differential Revision: D46700832 fbshipit-source-id: 7ac8699415d2bae2c8785d7ca96dfef0e1ba18a2

…or#5257) Summary: Pull Request resolved: facebookincubator#5257 In previous PR: facebookincubator#3417 (D46169043), the buck TARGETS file changes are overlooked. Thus it causes many internal build failures. This change is to amend those BUCK changes. Reviewed By: xiaoxmeng Differential Revision: D46700832 fbshipit-source-id: d4c6fed12163032f2739f72f72cf4447d5917412

…or#5257) Summary: Pull Request resolved: facebookincubator#5257 In previous PR: facebookincubator#3417 (D46169043), the buck TARGETS file changes are overlooked. Thus it causes many internal build failures. This change is to amend those BUCK changes. Reviewed By: xiaoxmeng Differential Revision: D46700832 fbshipit-source-id: 0a5ca6282152bd56e3111b87550dd6cf5ae9a980

Summary: Pull Request resolved: #5257 In previous PR: #3417 (D46169043), the buck TARGETS file changes are overlooked. Thus it causes many internal build failures. This change is to amend those BUCK changes. Reviewed By: xiaoxmeng Differential Revision: D46700832 fbshipit-source-id: 0e0e6833a8bead9dc4fccb5e08564ee0b8f7dae1

…okincubator#3418 What changes were proposed in this pull request? (Please fill in changes proposed in this fix) Fixes: facebookincubator#3417 How was this patch tested? (Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests) unit tests (If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 5, 2022

darrenfu marked this pull request as draft December 6, 2022 08:53

darrenfu mentioned this pull request Dec 9, 2022

Add Presto functions #2262

Open

darrenfu marked this pull request as ready for review December 12, 2022 18:23

darrenfu requested a review from mbasmanova December 12, 2022 18:46

mbasmanova requested changes Dec 14, 2022

View reviewed changes

velox/docs/functions/map.rst Outdated Show resolved Hide resolved

velox/functions/prestosql/tests/MapFromEntriesTest.cpp Show resolved Hide resolved

darrenfu requested a review from mbasmanova December 15, 2022 07:15

mbasmanova reviewed Dec 15, 2022

View reviewed changes

mbasmanova requested changes Dec 16, 2022

View reviewed changes

darrenfu force-pushed the map_from_entries_simpleudf branch from e316a31 to a584123 Compare December 21, 2022 02:32

darrenfu mentioned this pull request Dec 21, 2022

[fuzzer] One path fails on map_from_entries() #3581

Closed

mbasmanova reviewed Jan 4, 2023

View reviewed changes

velox/functions/prestosql/MapFromEntries.cpp Outdated Show resolved Hide resolved

velox/functions/prestosql/MapFromEntries.cpp Show resolved Hide resolved

velox/functions/prestosql/MapFromEntries.cpp Outdated Show resolved Hide resolved

mbasmanova reviewed Jan 5, 2023

View reviewed changes

velox/docs/functions/json.rst Outdated Show resolved Hide resolved

darrenfu mentioned this pull request Jan 11, 2023

isOneLevelDictionary(wrapper) in DecodedVector.cpp doesn't consider some 3+ levels of encodings #3687

Closed

darrenfu force-pushed the map_from_entries_simpleudf branch 2 times, most recently from 12c4767 to f321a5e Compare January 11, 2023 23:04

darrenfu force-pushed the map_from_entries_simpleudf branch from f321a5e to 2653f6e Compare February 7, 2023 00:06

darrenfu force-pushed the map_from_entries_simpleudf branch from f9a6a7d to fe1f06a Compare June 2, 2023 06:15

darrenfu marked this pull request as draft June 2, 2023 06:15

darrenfu force-pushed the map_from_entries_simpleudf branch from fe1f06a to 015c199 Compare June 12, 2023 06:59

darrenfu requested a review from kagamiori June 12, 2023 06:59

darrenfu force-pushed the map_from_entries_simpleudf branch from 015c199 to dcad9a3 Compare June 12, 2023 07:01

darrenfu marked this pull request as ready for review June 12, 2023 07:01

darrenfu force-pushed the map_from_entries_simpleudf branch from dcad9a3 to 60e3240 Compare June 12, 2023 07:04

pranjalssh approved these changes Jun 12, 2023

View reviewed changes

xiaoxmeng reviewed Jun 13, 2023

View reviewed changes

facebook-github-bot closed this in 0e6eb8f Jun 13, 2023

facebook-github-bot added the Merged label Jun 13, 2023

facebook-github-bot added the Reverted label Jun 13, 2023

darrenfu mentioned this pull request Jun 13, 2023

Add buck support to map_from_entries Presto function #5257

Closed

Add map_from_entries Presto function #3417

Add map_from_entries Presto function #3417

Conversation

darrenfu commented Dec 5, 2022

Summary

netlify bot commented Dec 5, 2022 • edited Loading

✅ Deploy Preview for meta-velox canceled.

mbasmanova commented Dec 12, 2022

darrenfu commented Dec 13, 2022

mbasmanova left a comment

Choose a reason for hiding this comment

mbasmanova commented Dec 15, 2022 • edited Loading

mbasmanova left a comment

Choose a reason for hiding this comment

darrenfu commented Dec 16, 2022

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darrenfu Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darrenfu Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darrenfu commented Dec 21, 2022

bikramSingh91 commented Dec 21, 2022

kagamiori commented Dec 21, 2022

bikramSingh91 commented Dec 21, 2022

mbasmanova commented Jan 4, 2023

bikramSingh91 commented Jan 4, 2023

mbasmanova left a comment

Choose a reason for hiding this comment

mbasmanova commented Jan 10, 2023

darrenfu commented May 26, 2023

kgpai commented May 26, 2023

darrenfu commented May 26, 2023

darrenfu commented Jun 12, 2023

facebook-github-bot commented Jun 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 13, 2023

conbench-facebook bot commented Jun 13, 2023

facebook-github-bot commented Jun 13, 2023

netlify bot commented Dec 5, 2022 •

edited

Loading

mbasmanova commented Dec 15, 2022 •

edited

Loading

darrenfu Dec 16, 2022 •

edited

Loading

darrenfu Jan 11, 2023 •

edited

Loading