[Substrait-to-Velox] Capture the file format specified in Substrait plan #1683

JkSelf · 2022-05-24T05:38:20Z

No description provided.

facebook-github-bot · 2022-05-24T05:38:24Z

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

mbasmanova

@JkSelf Would you explain what is the problem you are fixing and what is the solution? Storing file format, starts, lengths, etc. in the plan converter is strange and won't work when there are multiple file formats. Also, please, add a test.

JkSelf · 2022-05-24T09:13:06Z

@mbasmanova

Thanks for your review!

This PR is follow-up of #1048. We need the file format, starts, lengths, etc. when creating the HiveConnectorSplit for source operator. This PR can specific the file format in HiveConnectorSplit and then calling the corresponding reader to read the file. I will add tests later.

mbasmanova · 2022-05-24T09:59:25Z

We need the file format, starts, lengths, etc. when creating the HiveConnectorSplit for source operator.

Perhaps, one option is to define a struct to hold split information and store a mapping from plan node ID to a list of splits.

facebook-github-bot · 2022-05-25T00:18:03Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

JkSelf · 2022-05-26T02:08:23Z

Perhaps, one option is to define a struct to hold split information and store a mapping from plan node ID to a list of splits.

Agree with your suggestions. I have updated and please help to review again.

JkSelf · 2022-05-26T02:09:47Z

@rui-mo Please help to review. Thanks for your help!

mbasmanova

@JkSelf Looks good. A few comments.

mbasmanova · 2022-05-26T02:12:18Z

velox/substrait/SubstraitToVeloxPlan.h

@@ -25,6 +25,35 @@ namespace facebook::velox::substrait {
 /// This class is used to convert the Substrait plan into Velox plan.
 class SubstraitVeloxPlanConverter {
 public:
+  struct SplitStats {


naming: Since there are not statistics, but rather information about the split, perhaps, rename to SplitInfo.

mbasmanova · 2022-05-26T02:12:33Z

velox/substrait/SubstraitToVeloxPlan.h

+        std::vector<std::string> paths,
+        std::vector<u_int64_t> starts,
+        std::vector<u_int64_t> lengths,
+        int fileFormat)


why int and not int32_t?

mbasmanova · 2022-05-26T02:13:13Z

velox/substrait/SubstraitToVeloxPlan.h

@@ -50,7 +79,8 @@ class SubstraitVeloxPlanConverter {
      u_int32_t& index,
      std::vector<std::string>& paths,


Consider changing this API to take a reference to SplitInfo struct to return a pair of PlanNodePtr and a SplitInfo

mbasmanova · 2022-05-26T02:14:05Z

velox/substrait/SubstraitToVeloxPlan.h

-    return lengths_;
+  /// Return the splitStats map used by this plan converter.
+  const std::unordered_map<core::PlanNodeId, std::shared_ptr<SplitStats>>&
+  getSplitStatsMap() const {


the convention is not to add 'get' prefix to getters, e.g. splitInfos()

mbasmanova · 2022-05-26T02:14:25Z

velox/substrait/tests/Substrait2VeloxPlanConversionTest.cpp

@@ -46,21 +46,35 @@ class Substrait2VeloxPlanConversionTest
          const std::shared_ptr<const core::PlanNode>& planNode,
          const std::vector<std::string>& paths,
          const std::vector<u_int64_t>& starts,
-          const std::vector<u_int64_t>& lengths)
+          const std::vector<u_int64_t>& lengths,


use SplitInfo struct for readability

mbasmanova · 2022-05-26T02:15:54Z

velox/substrait/tests/Substrait2VeloxPlanConversionTest.cpp

@@ -136,9 +151,14 @@ class Substrait2VeloxPlanConversionTest
          facebook::velox::substrait::SubstraitVeloxPlanConverter>();
      // Convert to Velox PlanNode.
      auto planNode = planConverter->toVeloxPlan(substraitPlan, pool_.get());
+      auto splitStatsMap = planConverter->getSplitStatsMap();
+      auto leafPlanNodeIds = planNode->leafPlanNodeIds();
+      // Here only one leaf node is expected here.


perhaps, assert that there is exactly one node

mbasmanova · 2022-05-26T02:16:50Z

velox/substrait/tests/Substrait2VeloxPlanConversionTest.cpp

@@ -149,11 +169,11 @@ class Substrait2VeloxPlanConversionTest
        absolutePaths.emplace_back(fmt::format("{}{}", tempPath, path));
      }

-      std::vector<u_int64_t> starts = planConverter->getStarts();
-      std::vector<u_int64_t> lengths = planConverter->getLengths();
+      std::vector<u_int64_t> starts = splitStats->starts_;


This test is hard to read. Please, consider refactoring as in #1686

JkSelf · 2022-05-26T05:44:16Z

@mbasmanova
All comments have been updated except the test. There may be some repetitive work to refactor as in #1686. Can I update the test after #1686 is merged.

mbasmanova

@JkSelf Looks good. Perhaps, squash commits and rebase on top of 1720. A few small comments and it will be ready to land.

mbasmanova · 2022-05-26T12:16:29Z

velox/substrait/SubstraitToVeloxPlan.h

@@ -25,6 +25,25 @@ namespace facebook::velox::substrait {
 /// This class is used to convert the Substrait plan into Velox plan.
 class SubstraitVeloxPlanConverter {
 public:
+  struct SplitInfo {
+    SplitInfo() {}


Defining default constructor explicitly not necessary.

mbasmanova · 2022-05-26T12:17:21Z

velox/substrait/SubstraitToVeloxPlan.h

+    SplitInfo() {}
+
+    /// The Partition index.
+    u_int32_t partitionIndex_;


naming: members of the struct should not have underscore at the end, i.e. partitionIndex

mbasmanova · 2022-05-26T12:23:32Z

velox/substrait/SubstraitToVeloxPlan.cpp

-    lengths.reserve(fileList.size());
+    splitInfo->paths_.reserve(fileList.size());
+    splitInfo->starts_.reserve(fileList.size());
+    splitInfo->lengths_.reserve(fileList.size());
    for (const auto& file : fileList) {
      // Expect all Partitions share the same index.


Let's add a check to make sure this is indeed the case.

mbasmanova · 2022-05-26T12:24:43Z

velox/substrait/SubstraitToVeloxPlan.h

  /// The unique identification for each PlanNode.
  int planNodeId_ = 0;

  /// The map storing the relations between the function id and the function
  /// name. Will be constructed based on the Substrait representation.
  std::unordered_map<uint64_t, std::string> functionMap_;
+
+  /// The map storing the split stats for each PlanNode.


stats -> information

for each -> per

mbasmanova

@JkSelf Looks good % one question.

mbasmanova · 2022-06-06T15:36:24Z

velox/substrait/SubstraitToVeloxPlan.h

+    std::vector<u_int64_t> lengths;
+
+    /// The file format of the files to be scanned.
+    int32_t fileFormat;


What is the meaning of the integer here? Should we use dwio::common::FileFormat enum instead?

In the Substrait proto, I'm seeing file format defined as a struct:

https://github.com/substrait-io/substrait/blob/main/proto/substrait/algebra.proto#L118

oneof file_format { ParquetReadOptions parquet = 9; ArrowReadOptions arrow = 10; OrcReadOptions orc = 11; google.protobuf.Any extension = 12; }

oneof file_format { ParquetReadOptions parquet = 9; ArrowReadOptions arrow = 10; OrcReadOptions orc = 11; google.protobuf.Any extension = 12; }

The above code is newly committed in PR#169. And the FileFormat is described as follow in velox.

enum FileFormat{ FILE_FORMAT_UNSPECIFIED = 0; FILE_FORMAT_PARQUET = 1; }

I think we can change this after applying PR#169.

Got it. Let's change int32_t to dwio::common::FileFormat in this PR and update Substrait proto in a follow-up PR.

…t plan to velox plan

mbasmanova

@JkSelf Thank you for the contribution.

facebook-github-bot · 2022-06-10T04:48:39Z

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

…r#1683) Summary: Pull Request resolved: facebookincubator#1683 Reviewed By: amitkdutta Differential Revision: D37062244 Pulled By: mbasmanova fbshipit-source-id: 0177a0330040de4b357e721c7ea43a17b88605ba

…acebookincubator#1683) What changes were proposed in this pull request? This pr fix that if the current block is empty, it should return false for hasNext. (Fixes: facebookincubator#1682) How was this patch tested? This patch was tested manually.

mbasmanova reviewed May 24, 2022

View reviewed changes

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 May 25, 2022

mbasmanova reviewed May 26, 2022

View reviewed changes

JkSelf force-pushed the fileformat branch from 7776e5e to dbeb7d9 Compare June 6, 2022 01:00

mbasmanova reviewed Jun 6, 2022

View reviewed changes

JkSelf added 7 commits June 10, 2022 01:06

store fileformat of the files to be scanned when convert the substrai…

252d914

…t plan to velox plan

Refine the split info and add the unit test

a9c7142

resolve the comments

bcc6405

resolve the conflicts and comments

a3f4b0e

format style

8b30d22

format update

2fe5dbf

resolve the comments

691f915

JkSelf force-pushed the fileformat branch from 44d8471 to 691f915 Compare June 10, 2022 01:51

update the data path in test

e5957bc

mbasmanova approved these changes Jun 10, 2022

View reviewed changes

mbasmanova changed the title ~~Store file format of the files to be scanned when convert the substrait plan to velox plan~~ [Substrait-to-Velox] Capture the file format specified in Substrait plan Jun 10, 2022

facebook-github-bot closed this in f36d765 Jun 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Substrait-to-Velox] Capture the file format specified in Substrait plan #1683

[Substrait-to-Velox] Capture the file format specified in Substrait plan #1683

JkSelf commented May 24, 2022 •

edited by mbasmanova

Loading

facebook-github-bot commented May 24, 2022

mbasmanova left a comment

JkSelf commented May 24, 2022

mbasmanova commented May 24, 2022

facebook-github-bot commented May 25, 2022

JkSelf commented May 26, 2022

JkSelf commented May 26, 2022

mbasmanova left a comment

mbasmanova May 26, 2022

mbasmanova May 26, 2022

mbasmanova May 26, 2022

mbasmanova May 26, 2022

mbasmanova May 26, 2022

mbasmanova May 26, 2022

mbasmanova May 26, 2022

JkSelf commented May 26, 2022

mbasmanova left a comment

mbasmanova May 26, 2022

mbasmanova May 26, 2022

mbasmanova May 26, 2022

mbasmanova May 26, 2022

mbasmanova left a comment

mbasmanova Jun 6, 2022

JkSelf Jun 7, 2022

mbasmanova Jun 7, 2022

mbasmanova left a comment

facebook-github-bot commented Jun 10, 2022

		@@ -50,7 +79,8 @@ class SubstraitVeloxPlanConverter {
		u_int32_t& index,
		std::vector<std::string>& paths,

[Substrait-to-Velox] Capture the file format specified in Substrait plan #1683

[Substrait-to-Velox] Capture the file format specified in Substrait plan #1683

Conversation

JkSelf commented May 24, 2022 • edited by mbasmanova Loading

facebook-github-bot commented May 24, 2022

Action Required

Process

mbasmanova left a comment

Choose a reason for hiding this comment

JkSelf commented May 24, 2022

mbasmanova commented May 24, 2022

facebook-github-bot commented May 25, 2022

JkSelf commented May 26, 2022

JkSelf commented May 26, 2022

mbasmanova left a 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

JkSelf commented May 26, 2022

mbasmanova left a 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

mbasmanova left a 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

mbasmanova left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 10, 2022

JkSelf commented May 24, 2022 •

edited by mbasmanova

Loading