[Substrait to Velox] Support selecting a subfield from struct #2451

rui-mo · 2022-09-02T08:23:32Z

This PR supports selecting a subfield from struct based on FieldAccessTypedExpr. Added a round-trip test.

netlify · 2022-09-02T08:23:40Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`0954625`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/64a38726154f4a0008fd9783

velox/substrait/SubstraitToVeloxExpr.cpp

zhejiangxiaomai · 2022-09-27T06:31:31Z

Hi, @mbasmanova Could you please take a look at this PR?

mbasmanova

@rui-mo @zhejiangxiaomai Some questions.

velox/substrait/VeloxToSubstraitType.cpp

velox/substrait/VeloxToSubstraitExpr.cpp

velox/substrait/tests/VeloxSubstraitRoundTripPlanConverterTest.cpp

velox/substrait/SubstraitToVeloxExpr.cpp

zhejiangxiaomai · 2022-10-13T07:30:48Z

Hi, @mbasmanova sorry for late reply. Could you please review again when you are free?

mbasmanova · 2022-10-13T10:12:12Z

@zhejiangxiaomai It has been a long time. CircleCI is failing. Would you take a look?

zhejiangxiaomai · 2022-10-13T10:14:03Z

@zhejiangxiaomai It has been a long time. CircleCI is failing. Would you take a look?

Yes, I am fixing it. The reason why we didn't reply was that we had a seven day holiday called National Day.

mbasmanova

@zhejiangxiaomai Some comments.

velox/substrait/tests/VeloxSubstraitRoundTripPlanConverterTest.cpp

velox/substrait/VeloxToSubstraitType.cpp

velox/substrait/VeloxToSubstraitExpr.cpp

velox/substrait/SubstraitToVeloxExpr.cpp

mbasmanova

There are open comments.

kgpai · 2023-01-03T18:43:16Z

Hi @zhejiangxiaomai , Feel free to pull from latest and address open comments if still interested . Thanks !

zhejiangxiaomai · 2023-01-04T06:19:10Z

Hi @zhejiangxiaomai , Feel free to pull from latest and address open comments if still interested . Thanks !

Thank you for reminding me. I am fixing some comments in this PR. I will pull latest commit for this PR.

zhejiangxiaomai · 2023-01-09T10:38:50Z

Hi @mbasmanova, I passed the UT for nested struct. Would you review this PR again? It's been a long time.
The main idea of this PR is that use nested StructField to represent fieldExpr. StructField need record the index of each layer.
For example,
{ "field": 0, "child": { "struct_field": { "field": 0, "child": { "struct_field": { "field": 1 } } } } }
will represent ROW["abc"]["ab"]["b"].

mbasmanova · 2023-01-09T14:33:22Z

@zhejiangxiaomai It has been a very long time indeed. I have things I need to work on today and tomorrow. I'll try to get to this PR sometime later this week.

zhejiangxiaomai · 2023-04-19T10:51:10Z

@zhejiangxiaomai Thank you. I have a few things I need to work on this week. I'll take a look early next week. Please, ping me if you don't hear from me by Tue morning.

Just warm reminder. @mbasmanova

mbasmanova

Some questions.

velox/substrait/SubstraitToVeloxExpr.cpp

mbasmanova · 2023-04-19T12:25:36Z

velox/substrait/SubstraitToVeloxExpr.cpp

+        }
+
+        inputColumnType = asRowType(inputColumnType->childAt(idx));
+        tmp = &tmp->child().struct_field();


Is tmp->child() guaranteed to be a direct reference? Where can I read about direct_reference?

struct_field() will return class StructField. StructField link

StructField child() will return ReferenceSegment struct_field.child() link

ReferenceSegment will represent one type of MapKey StructField ListElement. ReferenceSegment define

direct_reference is also ReferenceSegment. direct_reference link

mbasmanova · 2023-04-19T12:30:34Z

velox/substrait/VeloxToSubstraitExpr.cpp

+  // for a leaf in an expression, find idx from child by exprName.
+  // for a dereference expression, find idx from every child by exprName.
+  if (fieldExpr->isInputColumn() ||
+      std::dynamic_pointer_cast<const core::FieldAccessTypedExpr>(


Why do we need special processing for the case when input of FieldAccessTypedExpr is not FieldAccessTypedExpr? I expect we should process all cases where input is not null the same:

1- Convert input to substrait expression.
2- Wrap the converted expression in substrait dereference expression.

Do we have tests for both code paths ?

---- Do we have tests for both code paths ?
Yes, we have tests for both code paths.

case (cast(row_constructor(a, b) as row(a bigint, b bigint))).a is the case when input of FieldAccessTypedExpr is not FieldAccessTypedExpr.

case "(abc).ab.a" is the normal case.

Here is some context why I add std::dynamic_pointer_cast<const core::FieldAccessTypedExpr>(fieldExpr->inputs()[0]) == nullptr. I also think it is not normative.

There are two very similar cases.
First one is use row_constructor to create a row before ab.a. Second one is put cast and fieldAcess together.

## First case. auto plan = PlanBuilder() .values({data}) .project( {"cast(row_constructor(a, b) as row(a bigint, b bigint)) as ab"}) .project( {"ab.a"}) .planNode(); ## Second case. auto plan = PlanBuilder() .values({data}) .project( {"(cast(row_constructor(a, b) as row(a bigint, b bigint))).a"}) .planNode();

And their substrait plan are the same.

"direct_reference": { "struct_field": { "field": 0, "child": { "struct_field": { "field": 0 } } }

But there is a difference for two cases that is the inputType for function getFieldIdForIntermediateNode.
First case inputType->toString() is ROW<ab:ROW<a:BIGINT,b:BIGINT>>.
But for the second case, I inputType->toString() is ROW<a:BIGINT,b:INTEGER,c:DOUBLE>. So I will got a null point core in getFieldIdForIntermediateNode function.

So, I think maybe the substrait plan of second case should like this.

"direct_reference": { "struct_field": { "field": 0, } }

which means we can ignore "cast" here. because we just want to find a from ROW<a:BIGINT,b:INTEGER,c:DOUBLE>. Do you think this is okay? if Ok, we should keep std::dynamic_pointer_cast<const core::FieldAccessTypedExpr>(fieldExpr->inputs()[0]) == nullptr logic. @mbasmanova

Now I found a better way to fix this condition. here

Because only input of FieldAccessTypedExpr is not FieldAccessTypedExpr will return struct_field. other condition will not. So it can be consider as find exprName from input directly. Do you think this is okay？@mbasmanova

zhejiangxiaomai · 2023-04-19T13:07:06Z

-- Some questions.
Thanks, I'll give some serious thought to these comments tomorrow.

zhejiangxiaomai · 2023-04-20T09:15:22Z

@mbasmanova Just fixed all comments.

velox/substrait/VeloxToSubstraitExpr.cpp

zhejiangxiaomai · 2023-04-24T08:50:33Z

Just warm reminder. @mbasmanova

zhejiangxiaomai · 2023-04-28T07:34:52Z

Hi, @mbasmanova do you have any suggestion? feel free to leave comments. 👍

mbasmanova · 2023-04-28T16:21:48Z

I'm short on bandwidth and confused by the changes in the PR.

@vibhatha Would you help review?

zhejiangxiaomai · 2023-05-04T02:53:47Z

Hi, @vibhatha do you have any suggestion?

vibhatha · 2023-05-04T03:05:13Z

I'm short on bandwidth and confused by the changes in the PR.

@vibhatha Would you help review?

@mbasmanova I will definitely take a look.

vibhatha · 2023-05-04T03:10:24Z

Hi, @vibhatha do you have any suggestion?

I will keep you posted. @zhejiangxiaomai

zhejiangxiaomai · 2023-05-15T02:48:39Z

Hi, @vibhatha do you have any suggestion?

I will keep you posted. @zhejiangxiaomai

Hi, @vibhatha please ping me. If you have any question. I will very happy to fix.

vibhatha · 2023-05-15T03:55:41Z

Hi, @vibhatha do you have any suggestion?

I will keep you posted. @zhejiangxiaomai

Hi, @vibhatha please ping me. If you have any question. I will very happy to fix.

Still looking into the code. Keep you posted as soon as possible.

zhejiangxiaomai · 2023-06-29T05:09:51Z

Hello,@vibhatha it's been a long time and I was wondering if you have any other suggestions? we already use this pr in our own repo.

2. support nested struct. like(abc.ab.a) The idea is use nested struct field to represet FieldAccessTypedExpr.

zhejiangxiaomai · 2023-07-04T02:45:26Z

@mbasmanova I'm very sorry to bother you, do you have any other suggestions? It seems like @vibhatha is too busy to review this PR.

mbasmanova · 2023-07-04T03:02:11Z

@zhejiangxiaomai Substrait PRs are very confusing to me. My understanding is that Substrait is designed to represent a logical plan (an alternative to SQL) and is not designed to represent optimized physical plan. It feels a bit hacky to use Substrait for representing optimized physical plan. Why not generate Velox plan directly instead? Velox plans now support serialization.

zhejiangxiaomai · 2023-07-04T03:25:03Z

@zhejiangxiaomai Substrait PRs are very confusing to me. My understanding is that Substrait is designed to represent a logical plan (an alternative to SQL) and is not designed to represent optimized physical plan. It feels a bit hacky to use Substrait for representing optimized physical plan. Why not generate Velox plan directly instead? Velox plans now support serialization.

Thanks, I will discuss with rui-mo.

zhejiangxiaomai · 2023-07-04T07:54:17Z

@zhejiangxiaomai Substrait PRs are very confusing to me. My understanding is that Substrait is designed to represent a logical plan (an alternative to SQL) and is not designed to represent optimized physical plan. It feels a bit hacky to use Substrait for representing optimized physical plan. Why not generate Velox plan directly instead? Velox plans now support serialization.

cc:@FelixYBW

FelixYBW · 2023-08-02T20:09:59Z

@rui-mo Let's close this one since we moved this part to Gluten.

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 Sep 2, 2022

rui-mo marked this pull request as draft September 2, 2022 08:30

zhejiangxiaomai force-pushed the sel_from_struct branch from 25e1651 to 3ab653c Compare September 26, 2022 02:41

rui-mo marked this pull request as ready for review September 26, 2022 02:47

rui-mo commented Sep 26, 2022

View reviewed changes

velox/substrait/SubstraitToVeloxExpr.cpp Outdated Show resolved Hide resolved

zhejiangxiaomai force-pushed the sel_from_struct branch from 3ab653c to 0931c31 Compare September 27, 2022 01:25

zhejiangxiaomai force-pushed the sel_from_struct branch from 0931c31 to 7db0780 Compare September 27, 2022 07:59

mbasmanova reviewed Sep 27, 2022

View reviewed changes

zhejiangxiaomai force-pushed the sel_from_struct branch from 7db0780 to 1bc8458 Compare October 13, 2022 07:22

zhejiangxiaomai force-pushed the sel_from_struct branch from 1bc8458 to b0de7e3 Compare October 13, 2022 08:17

zhejiangxiaomai force-pushed the sel_from_struct branch from b0de7e3 to 1e10e31 Compare October 13, 2022 10:20

mbasmanova approved these changes Oct 13, 2022

View reviewed changes

mbasmanova requested changes Oct 13, 2022

View reviewed changes

zhejiangxiaomai force-pushed the sel_from_struct branch 2 times, most recently from 79d5418 to 155b45a Compare October 17, 2022 05:11

zhejiangxiaomai force-pushed the sel_from_struct branch 4 times, most recently from 363b974 to e2e5481 Compare January 9, 2023 09:24

zhejiangxiaomai force-pushed the sel_from_struct branch 2 times, most recently from cb2ba77 to 1ee7455 Compare January 9, 2023 10:45

mbasmanova reviewed Apr 19, 2023

View reviewed changes

zhejiangxiaomai force-pushed the sel_from_struct branch from 1b2aeac to e19fc41 Compare April 20, 2023 07:04

zhejiangxiaomai force-pushed the sel_from_struct branch from e19fc41 to 8b8b886 Compare April 21, 2023 08:50

chaojun-zhang reviewed Apr 24, 2023

View reviewed changes

velox/substrait/VeloxToSubstraitExpr.cpp Outdated Show resolved Hide resolved

chaojun-zhang reviewed Apr 24, 2023

View reviewed changes

velox/substrait/VeloxToSubstraitExpr.cpp Outdated Show resolved Hide resolved

chaojun-zhang reviewed Apr 24, 2023

View reviewed changes

velox/substrait/VeloxToSubstraitExpr.cpp Outdated Show resolved Hide resolved

zhejiangxiaomai force-pushed the sel_from_struct branch from 8b8b886 to 18fae9d Compare April 24, 2023 05:18

zhejiangxiaomai force-pushed the sel_from_struct branch from 562a8d8 to c2d372a Compare April 28, 2023 07:36

rui-mo force-pushed the sel_from_struct branch from c2d372a to 6decec8 Compare June 6, 2023 01:39

rui-mo mentioned this pull request Jun 6, 2023

Support selecting a subfield from struct oap-project/velox#294

Merged

1. select from struct.

0954625

2. support nested struct. like(abc.ab.a) The idea is use nested struct field to represet FieldAccessTypedExpr.

zhejiangxiaomai force-pushed the sel_from_struct branch from 6decec8 to 0954625 Compare July 4, 2023 02:42

rui-mo closed this Aug 3, 2023

[Substrait to Velox] Support selecting a subfield from struct #2451

[Substrait to Velox] Support selecting a subfield from struct #2451

Uh oh!

Conversation

rui-mo commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

Uh oh!

zhejiangxiaomai commented Sep 27, 2022

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhejiangxiaomai commented Oct 13, 2022

Uh oh!

mbasmanova commented Oct 13, 2022

Uh oh!

zhejiangxiaomai commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

kgpai commented Jan 3, 2023

Uh oh!

zhejiangxiaomai commented Jan 4, 2023

Uh oh!

zhejiangxiaomai commented Jan 9, 2023

Uh oh!

mbasmanova commented Jan 9, 2023

Uh oh!

zhejiangxiaomai commented Apr 19, 2023

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mbasmanova Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

zhejiangxiaomai Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

mbasmanova Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

zhejiangxiaomai Apr 20, 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!

zhejiangxiaomai Apr 21, 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!

zhejiangxiaomai commented Apr 19, 2023

Uh oh!

zhejiangxiaomai commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

rui-mo commented Sep 2, 2022 •

edited

Loading

netlify bot commented Sep 2, 2022 •

edited

Loading

zhejiangxiaomai commented Oct 13, 2022 •

edited

Loading

zhejiangxiaomai Apr 20, 2023 •

edited

Loading

zhejiangxiaomai Apr 21, 2023 •

edited

Loading

zhejiangxiaomai commented Apr 20, 2023 •

edited

Loading