Fix Arrow convertor to honor dictionary encoding inside complex types. #7171

kgpai · 2023-10-20T21:48:20Z

Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type.

Reviewed By: panditsurabhi

Differential Revision: D50515953

netlify · 2023-10-20T21:48:25Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`fb2dcfe`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/655813943604b9000821876c

facebook-github-bot · 2023-10-20T21:48:30Z

This pull request was exported from Phabricator. Differential Revision: D50515953

Yuhta · 2023-10-23T22:23:53Z

velox/vector/arrow/Bridge.cpp

@@ -1114,7 +1115,13 @@ VectorPtr importFromArrowImpl(

  if (arrowSchema.dictionary) {
    return createDictionaryVector(
-        pool, type, nulls, arrowSchema, arrowArray, isViewer, wrapInBufferView);
+        pool,
+        INTEGER(),


We need to do some conversion if type is not INTEGER, cannot silently replace it.

Does does this function even takes type as a parameter if it only accepts INTEGER?

It can be expanded to support different widths of indices from arrow. In Velox we have only INTEGER, so some transformation needs to be done on the buffer depending on the type passed in.

To make sure I am on same page:

In arrow dictionaries can have different index types apart from integers ?

Since velox only supports integer indexes I will also make a change to createDictionary where we do the type conversion from whatever type arrow has to integer.

This helps : https://arrow.apache.org/docs/format/CDataInterface.html#dictionary-encoded-arrays .

FYI this diff doesnt add support for non integer index's - that should be in another diff.

pedroerp · 2023-11-03T21:05:13Z

velox/vector/arrow/Bridge.cpp

@@ -726,7 +726,8 @@ void exportToArrow(const VectorPtr& vec, ArrowSchema& arrowSchema) {
 }

 TypePtr importFromArrow(const ArrowSchema& arrowSchema) {
-  const char* format = arrowSchema.format;
+  const char* format = arrowSchema.dictionary ? arrowSchema.dictionary->format


can we have unit tests for this change. This should go under ArrowBridgeSchemaTest.cpp

pedroerp · 2023-11-03T21:07:54Z

velox/vector/arrow/Bridge.cpp

@@ -1114,7 +1115,13 @@ VectorPtr importFromArrowImpl(

  if (arrowSchema.dictionary) {
    return createDictionaryVector(
-        pool, type, nulls, arrowSchema, arrowArray, isViewer, wrapInBufferView);
+        pool,
+        INTEGER(),


Does does this function even takes type as a parameter if it only accepts INTEGER?

facebookincubator#7171) Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type. Differential Revision: D50515953

facebook-github-bot · 2023-11-13T22:54:54Z

This pull request was exported from Phabricator. Differential Revision: D50515953

facebookincubator#7171) Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type. Differential Revision: D50515953

facebook-github-bot · 2023-11-13T23:00:33Z

This pull request was exported from Phabricator. Differential Revision: D50515953

kgpai · 2023-11-13T23:12:18Z

cc: @pedroerp @Yuhta

facebookincubator#7171) Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type. Differential Revision: D50515953

facebook-github-bot · 2023-11-13T23:15:02Z

This pull request was exported from Phabricator. Differential Revision: D50515953

facebookincubator#7171) Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type. Differential Revision: D50515953

facebook-github-bot · 2023-11-13T23:46:13Z

This pull request was exported from Phabricator. Differential Revision: D50515953

facebookincubator#7171) Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type. Differential Revision: D50515953

facebook-github-bot · 2023-11-14T00:14:13Z

This pull request was exported from Phabricator. Differential Revision: D50515953

pedroerp · 2023-11-15T23:22:47Z

velox/vector/arrow/Bridge.cpp

-  }
-  VELOX_USER_FAIL(
-      "Unable to convert '{}' ArrowSchema format type to Velox.", format);
+  // As per


perhaps more clearly:

"For dictionaries, format encodes the index type, while the dictionary value is encoded in the dictionary member, as per: ..."

pedroerp · 2023-11-15T23:26:48Z

velox/vector/arrow/tests/ArrowBridgeSchemaTest.cpp

+      *testSchemaDictionaryImport(
+          "i",
+          makeComplexArrowSchema(
+              schemas, schemaPtrs, "+s", {"s", "f"}, {"col1", "col2"})));


can we have nested dictionary types (dict(dict(integer)))? If so, do we have tests for them?

facebookincubator#7171) Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type. Reviewed By: pedroerp Differential Revision: D50515953

facebook-github-bot · 2023-11-16T20:54:42Z

This pull request was exported from Phabricator. Differential Revision: D50515953

facebookincubator#7171) Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type. Reviewed By: pedroerp Differential Revision: D50515953

facebook-github-bot · 2023-11-16T20:56:28Z

This pull request was exported from Phabricator. Differential Revision: D50515953

facebookincubator#7171) Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type. Reviewed By: pedroerp Differential Revision: D50515953

facebook-github-bot · 2023-11-16T20:57:21Z

This pull request was exported from Phabricator. Differential Revision: D50515953

facebookincubator#7171) Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type. Reviewed By: pedroerp Differential Revision: D50515953

facebook-github-bot · 2023-11-17T18:48:32Z

This pull request was exported from Phabricator. Differential Revision: D50515953

facebookincubator#7171) Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type. Reviewed By: pedroerp Differential Revision: D50515953

facebook-github-bot · 2023-11-17T19:14:52Z

This pull request was exported from Phabricator. Differential Revision: D50515953

facebookincubator#7171) Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type. Reviewed By: pedroerp Differential Revision: D50515953

facebook-github-bot · 2023-11-17T21:09:17Z

This pull request was exported from Phabricator. Differential Revision: D50515953

facebookincubator#7171) Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type. Reviewed By: pedroerp Differential Revision: D50515953

facebook-github-bot · 2023-11-18T01:29:10Z

This pull request was exported from Phabricator. Differential Revision: D50515953

facebook-github-bot · 2023-11-18T01:30:10Z

This pull request was exported from Phabricator. Differential Revision: D50515953

facebook-github-bot · 2023-11-18T22:25:17Z

This pull request has been merged in a86cd7b.

conbench-facebook · 2023-11-18T22:48:26Z

Conbench analyzed the 1 benchmark run on commit a86cd7be.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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 Oct 20, 2023

facebook-github-bot added the fb-exported label Oct 20, 2023

Yuhta reviewed Oct 23, 2023

View reviewed changes

pedroerp reviewed Nov 3, 2023

View reviewed changes

kgpai force-pushed the export-D50515953 branch from ed0815b to 5182aa9 Compare November 13, 2023 22:54

kgpai force-pushed the export-D50515953 branch from 5182aa9 to d1b2d1e Compare November 13, 2023 23:00

kgpai force-pushed the export-D50515953 branch from d1b2d1e to 78b752e Compare November 13, 2023 23:14

kgpai force-pushed the export-D50515953 branch from 78b752e to db47886 Compare November 13, 2023 23:46

kgpai force-pushed the export-D50515953 branch from db47886 to c7bb3a2 Compare November 14, 2023 00:13

pedroerp approved these changes Nov 15, 2023

View reviewed changes

kgpai force-pushed the export-D50515953 branch from c7bb3a2 to b97c527 Compare November 16, 2023 20:54

kgpai force-pushed the export-D50515953 branch from b97c527 to 4c32261 Compare November 16, 2023 20:56

kgpai force-pushed the export-D50515953 branch from 4c32261 to a675126 Compare November 16, 2023 20:56

kgpai force-pushed the export-D50515953 branch from a675126 to 2f63a97 Compare November 17, 2023 18:48

kgpai force-pushed the export-D50515953 branch from 2f63a97 to 7182324 Compare November 17, 2023 19:14

kgpai force-pushed the export-D50515953 branch from 7182324 to ca8f1e2 Compare November 17, 2023 21:09

Fix Arrow convertor to honor dictionary encoding inside complex types. (

fb2dcfe

facebookincubator#7171) Summary: Arrow converter currently doesnt check for dictionary encoding etc when inside a complex type. Reviewed By: pedroerp Differential Revision: D50515953

kgpai force-pushed the export-D50515953 branch from ca8f1e2 to fb2dcfe Compare November 18, 2023 01:29

facebook-github-bot closed this in a86cd7b Nov 18, 2023

facebook-github-bot added the Merged label Nov 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Arrow convertor to honor dictionary encoding inside complex types. #7171

Fix Arrow convertor to honor dictionary encoding inside complex types. #7171

kgpai commented Oct 20, 2023

netlify bot commented Oct 20, 2023 •

edited

Loading

facebook-github-bot commented Oct 20, 2023

Yuhta Oct 23, 2023

pedroerp Nov 3, 2023

Yuhta Nov 7, 2023 •

edited

Loading

kgpai Nov 9, 2023

kgpai Nov 9, 2023

kgpai Nov 13, 2023

pedroerp Nov 3, 2023

pedroerp Nov 3, 2023

facebook-github-bot commented Nov 13, 2023

facebook-github-bot commented Nov 13, 2023

kgpai commented Nov 13, 2023

facebook-github-bot commented Nov 13, 2023

facebook-github-bot commented Nov 13, 2023

facebook-github-bot commented Nov 14, 2023

pedroerp Nov 15, 2023

pedroerp Nov 15, 2023

facebook-github-bot commented Nov 16, 2023

facebook-github-bot commented Nov 16, 2023

facebook-github-bot commented Nov 16, 2023

facebook-github-bot commented Nov 17, 2023

facebook-github-bot commented Nov 17, 2023

facebook-github-bot commented Nov 17, 2023

facebook-github-bot commented Nov 18, 2023

facebook-github-bot commented Nov 18, 2023

facebook-github-bot commented Nov 18, 2023

conbench-facebook bot commented Nov 18, 2023

Fix Arrow convertor to honor dictionary encoding inside complex types. #7171

Fix Arrow convertor to honor dictionary encoding inside complex types. #7171

Conversation

kgpai commented Oct 20, 2023

netlify bot commented Oct 20, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuhta Nov 7, 2023 • edited Loading

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 Nov 13, 2023

facebook-github-bot commented Nov 13, 2023

kgpai commented Nov 13, 2023

facebook-github-bot commented Nov 13, 2023

facebook-github-bot commented Nov 13, 2023

facebook-github-bot commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 16, 2023

facebook-github-bot commented Nov 16, 2023

facebook-github-bot commented Nov 16, 2023

facebook-github-bot commented Nov 17, 2023

facebook-github-bot commented Nov 17, 2023

facebook-github-bot commented Nov 17, 2023

facebook-github-bot commented Nov 18, 2023

facebook-github-bot commented Nov 18, 2023

facebook-github-bot commented Nov 18, 2023

conbench-facebook bot commented Nov 18, 2023

netlify bot commented Oct 20, 2023 •

edited

Loading

Yuhta Nov 7, 2023 •

edited

Loading