[Substrait to Velox] add avg support based on row type

[zhejiangxiaomai]

• Add support for converting Substrait struct type into Velox ROW type.
• Add support for expressions using row_constructor function.
• Update JSON files for q1 and q6. (remove function "name": "avg:opt_fp64". use sum and count to replace it)

[zhejiangxiaomai]

@zhejiangxiaomai These changes are hard to follow. Would it be possible to split this PR in to two: add support for aggregations + add support for q6. It would be nice to add a round-trip test for aggregation.

There are several tests in VeloxSubstraitRoundTripPlanConverterTest.cpp about aggregation. Maybe I need add a case about avg?

[zhejiangxiaomai]

Hi, @mbasmanova In my opinion, all comments were fixed. Do you review again? and merge it.

[zhejiangxiaomai]

@zhejiangxiaomai The PR title says "add support for aggregations", which suggests that without the changes in the PR plans with aggregations were not supported. Is this the case? If so, I'd expect this PR to include a unit test that verifies conversion of a plan with an aggregation, but there are no tests here. Please, clarify.

@mbasmanova Sorry for misunderstanding, I changed the title of this PR. Actually, this PR just make velox can work at avg() case based on row_constructor. And I will add UT that convert a json which contains avg to some velox plan nodes, and verify it.

[mbasmanova]

@zhejiangxiaomai Thank you for clarify the PR title. It reads "support avg function based on row_construct" now. Would you also update the PR description to explain what does that mean to support avg function based on row_construct. Re: testing - let's write a round-trip tests instead of relying on checked-in JSON files. These files are very hard to read and therefore tests are difficult to understand. Let's discuss more in Slack.

[zhejiangxiaomai]

@zhejiangxiaomai Thank you for clarify the PR title. It reads "support avg function based on row_construct" now. Would you also update the PR description to explain what does that mean to support avg function based on row_construct. Re: testing - let's write a round-trip tests instead of relying on checked-in JSON files. These files are very hard to read and therefore tests are difficult to understand. Let's discuss more in Slack.

OK, a round-trip test will add.

[zhejiangxiaomai]

Hi,@mbasmanova I think all comments were fixed, could you please review it again when you have time? Actually, I have to clarify some points about this PR.

1. A round-trip test are planned to add here, but we found transfer "row_construct" to substrait plan is not supported now when adding some cases on my local branch. And @ZJie1 will help me to support it soon, then round-trip will be added. So I'm sorry for not add round-trip. Can i add a one-way test？ which means a test case get substrait plan from json about row_construct, and call toVeloxPlan to verify result.
2. Json update is necessary in this PR , because plan is changed, I have to add it with code together. Then the UT test can pass. Although it make this PR looks too bloated to review.
3. hard-code is not existed, all code is move to toVeloxType function.

[zhejiangxiaomai]

@zhejiangxiaomai Looks good overall % some comments below.

we found transfer "row_construct" to substrait plan is not supported now

I'm curious what's the problem with supporting row_constructor. How hard would it be to fix this?

Hi, @mbasmanova,I try to use row_constructor to make a plan which contains "avg" or "filter", but it needs more code to support at "velox to substrait" side and "substrait to velox" side. For example, "avg" is not supported at "velox to substrait" side. And I deem that changes should not be added in this PR. The good news is single row_construct supported by me and @ZJie1 in this PR. And I add a round-trip test named "row_constructor" in VeloxSubstraitRoundTripPlanConverterTest.cpp.

More UT test cases are added to verify logic supports nested structs. So could you please review it again?

[mbasmanova]

@zhejiangxiaomai I spent some time running and debugging Substrait conversion code.

First, I ran into this error:

error: 'args' is deprecated [-Werror,-Wdeprecated-declarations]

./velox/substrait/proto/substrait/algebra.pb.h:11241:3: note: 'args' has been explicitly marked deprecated here
  PROTOBUF_DEPRECATED const ::PROTOBUF_NAMESPACE_ID::RepeatedPtrField< ::substrait::Expression >&
  ^
/usr/local/include/google/protobuf/port_def.inc:310:45: note: expanded from macro 'PROTOBUF_DEPRECATED'
# define PROTOBUF_DEPRECATED __attribute__((deprecated))

I updated the code to start using arguments field instead of args. The code compiles, but a test that reads q6_first_stage.json fails because the JSON doesn't have 'arguments' field. I wonder how to update the JSON file.

Next, I realized that currently the converter is lacking support for ROW type. This type is used for example in queries with avg aggregation because avg uses ROW type for the intermediate result type.

Here is a simple test to illustrate the lack of support for ROW type.

TEST_F(VeloxSubstraitRoundTripPlanConverterTest, avg) {
  auto vectors = makeVectors(2, 7, 3);
  createDuckDbTable(vectors);

  auto plan = PlanBuilder()
                  .values(vectors)
                  .partialAggregation({}, {"avg(c4)"})
                  .finalAggregation()
                  .planNode();

  assertPlanConversion(plan, "SELECT avg(c4) FROM tmp");
}

I then extracted minimum changes from this PR to add support for ROW type and allow the 'avg' test above to pass.

Here are all the changes I made: https://github.com/facebookincubator/velox/pull/2264

I propose the following.

1. Make a PR to fix "error: 'args' is deprecated [-Werror,-Wdeprecated-declarations]".
2. Make another PR to add support for ROW type and test it using the above 'avg' test.
3. Make more PRs for other changes in this PR.

What do you think?

[zhejiangxiaomai]

@zhejiangxiaomai Looks good overall % a couple of questions.

I find it difficult to understand the PR title and description and wonder if there is a way to clarify these a bit.

Based on my understanding of the changes in this PR, I suggest the following PR title and description:

Title:

[Substrait to Velox] Add support for struct types

Description:

* Add support for converting Substrait struct type into Velox ROW type.
* Add support for expressions using row_constructor function.
* Update JSON files for q1 and q6 to <TODO: describe plan changes>

I find changes in JSON files unreadable, hence, cannot figure out what are the changes. Any chance you could explain these?

Tittle of PR has been changed. I explain the change of json file is that function name avg was replaced by count.

[zhejiangxiaomai]

@zhejiangxiaomai I spent some time running and debugging Substrait conversion code.

First, I ran into this error:

error: 'args' is deprecated [-Werror,-Wdeprecated-declarations]

./velox/substrait/proto/substrait/algebra.pb.h:11241:3: note: 'args' has been explicitly marked deprecated here
  PROTOBUF_DEPRECATED const ::PROTOBUF_NAMESPACE_ID::RepeatedPtrField< ::substrait::Expression >&
  ^
/usr/local/include/google/protobuf/port_def.inc:310:45: note: expanded from macro 'PROTOBUF_DEPRECATED'
# define PROTOBUF_DEPRECATED __attribute__((deprecated))

I updated the code to start using arguments field instead of args. The code compiles, but a test that reads q6_first_stage.json fails because the JSON doesn't have 'arguments' field. I wonder how to update the JSON file.

Next, I realized that currently the converter is lacking support for ROW type. This type is used for example in queries with avg aggregation because avg uses ROW type for the intermediate result type.

Here is a simple test to illustrate the lack of support for ROW type.

TEST_F(VeloxSubstraitRoundTripPlanConverterTest, avg) {
  auto vectors = makeVectors(2, 7, 3);
  createDuckDbTable(vectors);

  auto plan = PlanBuilder()
                  .values(vectors)
                  .partialAggregation({}, {"avg(c4)"})
                  .finalAggregation()
                  .planNode();

  assertPlanConversion(plan, "SELECT avg(c4) FROM tmp");
}

I then extracted minimum changes from this PR to add support for ROW type and allow the 'avg' test above to pass.

Here are all the changes I made: #2264

I propose the following.

1. Make a PR to fix "error: 'args' is deprecated [-Werror,-Wdeprecated-declarations]".
2. Make another PR to add support for ROW type and test it using the above 'avg' test.
3. Make more PRs for other changes in this PR.

What do you think?

Yes, I approved. your propose is definitely right. I will make a PR to fix "args error" firstly. Then we can merge #2264 and other changes in this pr. In addition, your supports on row-type PR is perfect. Json file is a little bit difficult to produce. I will produce it by gluten + spark.

[mbasmanova]

@zhejiangxiaomai

I will make a PR to fix "args error"

That will be awesome. Thank you!

Then we can merge #2264

I'll work on getting #2264 reviewed and merged. We can build on top of it.

[zhejiangxiaomai]

@zhejiangxiaomai

I will make a PR to fix "args error"

That will be awesome. Thank you!

Then we can merge #2264

I'll work on getting #2264 reviewed and merged. We can build on top of it.

Hi, @mbasmanova I know the reason why you met "args error" problem. Because your personal environment protobuf version is higher than 3.6. you can use "protoc --version" to check. But In "setup-ubuntu.sh" script, it use "apt install -y protobuf-compiler " and the corresponding version of protobuf is 3.6. So github circleci can still work.

I recommand that we should upgrade protobuf to a higher version like v3.21.4. Then I can refresh the json file.

[mbasmanova]

@zhejiangxiaomai You are correct. I have protoc version 3.19.4.

% protoc --version
libprotoc 3.19.4

I recommand that we should upgrade protobuf to a higher version like v3.21.4

Sounds reasonable. Would you like to send out a PR?

CC: @kgpai @mshang816 @majetideepak

[zhejiangxiaomai]

@zhejiangxiaomai You are correct. I have protoc version 3.19.4.

% protoc --version
libprotoc 3.19.4

I recommand that we should upgrade protobuf to a higher version like v3.21.4

Sounds reasonable. Would you like to send out a PR?

CC: @kgpai @mshang816 @majetideepak

Hi @mbasmanova , https://github.com/facebookincubator/velox/pull/2383 upgrade protobuf version, and fix "args" problem. Could you please review it?

[netlify[bot]]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	e4f74c75faeafd867d5a307ffce9cbbc8ae1a8ad
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/632463906f75b000092763f8

[zhejiangxiaomai]

Hi, @mbasmanova, round-trip tests are added, Could you please review this PR again? Thank you.

[zhejiangxiaomai]

Hi @mbasmanova, I fix json error which is detect by substrait validator. can you take a look, and merge this PR. Here is result of q6_first_stage.json.

[zhejiangxiaomai]

Here is the substrait-validator result of q1_first_stage.json.

[facebook-github-bot]

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