[VL shuffle] Add VeloxSplitter to split with velox::RowVector by zuochunwei · Pull Request #1003 · apache/gluten

zuochunwei · 2023-02-22T08:12:22Z

What changes were proposed in this pull request?

add VeloxSplitter to support splitting with velox::RowVector. when the backend is Velox, splitting with RowVector can avoid transferring from velox::RowVector to arrow::RowBatch, it's more efficient.
we should avoid writing using namespace arrow or using namespace facebook::velox in header files, this usage brings mess and conflict.
I will add Unit test soon, so file cpp/velox/tests/VeloxSplitterTest.cc is not ready now, but it will not affect others.
I will also add the JNI call functions in file velox/jni/JniWrapper soon.

(Please fill in changes proposed in this fix)

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)

(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

github-actions · 2023-02-22T08:12:41Z

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/oap-project/gluten/issues

Then could you also rename commit message and pull request title in the following format?

[Gluten-${ISSUES_ID}] ${detailed message}

See also:

Other pull requests

jinchengchenghh · 2023-02-22T08:44:09Z

Please change the remark to english

jinchengchenghh · 2023-02-22T08:44:51Z

Can you use the benchmark https://github.com/oap-project/gluten/blob/main/cpp/core/benchmarks/ShuffleSplitBenchmark.cc to test if it works better than old shuffle?

jinchengchenghh · 2023-02-22T08:46:42Z

Maybe you can use using namespace arrow in this .cc file

zuochunwei · 2023-02-22T09:28:11Z

Can you use the benchmark https://github.com/oap-project/gluten/blob/main/cpp/core/benchmarks/ShuffleSplitBenchmark.cc to test if it works better than old shuffle?

The Unit test is not completed yet, I will run bench-mark after I complete the unit-test

zhztheplayer · 2023-02-22T12:41:41Z

Will Arrow IPC writer be used in velox splitter? or probably the headers need to be minimized

OK, I will check it, and we should minimize the header files to include

yes, structs such as arrow::ipc::IpcPayload and arrow::ipc::IpcWriteOptions are used, so including these header files is necessary.

zhztheplayer · 2023-02-22T12:48:02Z

I don't have much context here but my intuition is that the code can base on row vector's schema instead of Arrow schema. But current way is also OK to me as of now.

I don't have much context here but my intuition is that the code can base on row vector's schema instead of Arrow schema. But current way is also OK to me as of now.

In my first implementation, I tried to use Row Vector‘s schema, but I found it was not a good way because too many changes are required.

I get the arrow::Schema from RowVector using the interface velox::exportToArrow provided by Velox, and then it works well with RowVector. otherwise, I will have to switch case every Velox DataType and be careful of the relations of the two type systems.

OK. So let's just keep the current way, it's not a big deal at this time.

jinchengchenghh · 2023-02-23T03:21:08Z

Can we get arrow Schema here?

I kept this interface here, after I add the code for JNI, I will delete it if I find that it is useless. OK?

zuochunwei · 2023-02-23T09:55:21Z

@zzcclp would you help me to check the problem of CH Unit-Test for this PR

FelixYBW · 2023-02-23T21:20:41Z

There is a shuffle benchmark here: cpp/core/benchmarks/ShuffleSplitBenchmark.cc. Current the benchmark read parquet by arrow then do the slit. You may modify it as read parquet by velox, then test velox->arrow + arrow split vs. velox split to arrow. We may check the perf gain and optimize the velox split code.

zuochunwei · 2023-02-24T01:43:16Z

There is a shuffle benchmark here: cpp/core/benchmarks/ShuffleSplitBenchmark.cc. Current the benchmark read parquet by arrow then do the slit. You may modify it as read parquet by velox, then test velox->arrow + arrow split vs. velox split to arrow. We may check the perf gain and optimize the velox split code.

got it, I will append the benchmark

zjuwangg · 2023-08-10T09:49:07Z

+#endif
+
+  // partition number should be less than 64k
+  ARROW_CHECK_LE(num_partitions_, 64 * 1024);


why num_partitions_ can not be greater then 64K?

Because the partition id is 16bit to save memory footprint. If your case needs more than 64K reducers, feel free to add 32bit support through template.

ulysses-you · 2023-09-19T12:22:20Z

+  // subscript: Row ID
+  // value: Partition ID
+  // TODO: rethink, is uint16_t better?
+  std::vector<uint32_t> row_2_partition_; // note: partition_id_


why naming these fields as xxx_2_xxx ? it is not a map, why not just partitionIds ?

Feel free to update for more reasonable names.

zuochunwei force-pushed the ss branch from 1288515 to 22bf68e Compare February 22, 2023 08:31

jinchengchenghh reviewed Feb 22, 2023

View reviewed changes

zuochunwei force-pushed the ss branch from 17fc354 to 9ec49b6 Compare February 22, 2023 09:23

FelixYBW added the velox backend works for Velox backend label Feb 22, 2023

FelixYBW changed the title ~~[shuffle] Add VeloxSplitter to split with velox::RowVector~~ [VL shuffle] Add VeloxSplitter to split with velox::RowVector Feb 22, 2023

zuochunwei force-pushed the ss branch from 61fd57a to 32fa5a9 Compare February 22, 2023 09:44

zhztheplayer reviewed Feb 22, 2023

View reviewed changes

jinchengchenghh reviewed Feb 23, 2023

View reviewed changes

FelixYBW removed the velox backend works for Velox backend label Feb 23, 2023

zuochunwei force-pushed the ss branch from 9aebe3f to b459239 Compare February 23, 2023 06:27

zuochunwei force-pushed the ss branch from 43b3e4d to 4df67ec Compare February 23, 2023 11:43

zuochunwei force-pushed the ss branch from 1017da9 to 6a19311 Compare February 24, 2023 02:29

add Velox shuffle splitter

c9fd76d

zuochunwei force-pushed the ss branch from 6a19311 to c9fd76d Compare February 24, 2023 05:52

bypass unit test

e391596

jinchengchenghh approved these changes Feb 24, 2023

View reviewed changes

zuochunwei merged commit 746154d into apache:main Feb 24, 2023

zuochunwei mentioned this pull request Feb 27, 2023

[VL] Add Unit-Test for VeloxSplitter #1041

Merged

zuochunwei deleted the ss branch March 23, 2023 09:00

zjuwangg reviewed Aug 10, 2023

View reviewed changes

ulysses-you reviewed Sep 19, 2023

View reviewed changes

Conversation

zuochunwei commented Feb 22, 2023

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

github-actions Bot commented Feb 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuochunwei Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinchengchenghh commented Feb 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuochunwei commented Feb 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuochunwei Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuochunwei Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuochunwei commented Feb 23, 2023

Uh oh!

FelixYBW commented Feb 23, 2023

Uh oh!

zuochunwei commented Feb 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zuochunwei Feb 22, 2023 •

edited

Loading

zuochunwei Feb 23, 2023 •

edited

Loading

zuochunwei Feb 23, 2023 •

edited

Loading