refactor: always insert a exchange singleton on top dml

[BowenXiao1999]

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

Currently only a demo and I'm not sure whether which way we should do.

Checklist

• I have written necessary rustdoc comments
• I have added necessary unit tests and integration tests
• All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

If your pull request contains user-facing changes, please specify the types of the changes, and create a release note. Otherwise, please feel free to remove this section.

Types of user-facing changes

Please keep the types that apply to your changes, and remove those that do not apply.

• Installation and deployment
• Connector (sources & sinks)
• SQL commands, functions, and operators
• RisingWave cluster configuration changes
• Other (please specify in the release note below)

Release note

Please create a release note for your changes. In the release note, focus on the impact on users, and mention the environment or conditions where the impact may occur.

Refer to a related PR or issue link (optional)

[BowenXiao1999]

After #4708, now we are able to distribute dml by faciliate it as MPP. However, according to #3269 , I'm working on to execute root fragment of MPP in FE, while Insert / Update / Delete now can not execute in FE (?), so this is a blocked of #4688. I plan to hack and insert a Exchange Singleton on dml to solve this.

This PR shows what will be, I think the dirty is controllable for now.

cc @chenzl25 @liurenjie1024 @st1page @fuyufjh , What's your opinion?

[liurenjie1024]

LGTM.

[chenzl25]

+1

[fuyufjh]

I plan to hack and insert a Exchange Singleton on dml to solve this.

I would like to leave the top node distributed and support this kind of plan in scheduler

[BowenXiao1999]

I plan to hack and insert a Exchange Singleton on dml to solve this.

I would like to leave the top node distributed and support this kind of plan in scheduler

Update / Delete / Insert can be distributed if we support it, Add a singleton exchange will not change that, cuz previously we use get_data exchange service RPC to fetch results, now we add a exchange locally and no network cost introduced.

if Update / Delete / Insert is distributed, we still need a root singleton to get merged reported results. If currently we do not want to add this singleton, then we can not execute it in FE, unless we hack in scheduler that "unlike DQL, this kind of query (DML) will schedule root fragment to CN ", which I think may be more dirty.

[st1page]

LGTM

[fuyufjh]

I plan to hack and insert a Exchange Singleton on dml to solve this.

I would like to leave the top node distributed and support this kind of plan in scheduler

Update / Delete / Insert can be distributed if we support it, Add a singleton exchange will not change that, cuz previously we use get_data exchange service RPC to fetch results, now we add a exchange locally and no network cost introduced.

if Update / Delete / Insert is distributed, we still need a root singleton to get merged reported results. If currently we do not want to add this singleton, then we can not execute it in FE, unless we hack in scheduler that "unlike DQL, this kind of query (DML) will schedule root fragment in CN ", which I think may be more dirty.

OK, make sense to me

[BowenXiao1999]

Seems like encounter some problems relates to e2e deterministic test, construct a minimal example in #4788. May wait to see any progress.

[liurenjie1024]

Still not working? I think @wangrunji0408 has resolved the issue in madsim?

[wangrunji0408]

Still not working? I think @wangrunji0408 has resolved the issue in madsim?

Yes. But this looks like a new problem in parallel e2e:

thread '<unnamed>' panicked at 'index out of bounds: the len is 0 but the index is 0', src/frontend/src/handler/dml.rs:50:29

[BowenXiao1999]

Still not working? I think @wangrunji0408 has resolved the issue in madsim?

Yes. But this looks like a new problem in parallel e2e:

thread '<unnamed>' panicked at 'index out of bounds: the len is 0 but the index is 0', src/frontend/src/handler/dml.rs:50:29

I will take a look at this. Tested success for serialize e2e locally.

[liurenjie1024]

I think we may need to add some flush commands in parallel e2e to support dml.

[BowenXiao1999]

Still not working? I think @wangrunji0408 has resolved the issue in madsim?

The root cause of this problem is: #4947 . Errors may happen after QueryResultFetcher starts to work. I add a if-statement to workaround this, only for this PR.

[skyzh]

removed can-merge as it's a draft

[codecov]

Codecov Report

Merging #4752 (8ea3111) into main (a14d883) will decrease coverage by 0.08%.
The diff coverage is 48.62%.

@@            Coverage Diff             @@
##             main    #4752      +/-   ##
==========================================
- Coverage   74.21%   74.12%   -0.09%     
==========================================
  Files         882      883       +1     
  Lines      134837   135315     +478     
==========================================
+ Hits       100069   100304     +235     
- Misses      34768    35011     +243     

Flag	Coverage Δ
rust	74.12% <48.62%> (-0.09%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/batch/src/executor/hash_agg.rs	93.08% <ø> (-0.03%)	⬇️
src/common/src/error.rs	72.19% <ø> (ø)
src/compute/src/rpc/service/stream_service.rs	0.00% <0.00%> (ø)
src/compute/src/server.rs	0.00% <0.00%> (ø)
src/expr/src/error.rs	25.00% <ø> (ø)
src/expr/src/expr/build_expr_from_prost.rs	55.51% <0.00%> (-2.48%)	⬇️
src/expr/src/expr/expr_to_char_const_tmpl.rs	0.00% <0.00%> (ø)
src/expr/src/expr/mod.rs	48.93% <ø> (ø)
src/expr/src/vector_op/to_char.rs	0.00% <0.00%> (ø)
src/frontend/src/handler/create_source.rs	78.94% <0.00%> (-14.81%)	⬇️
... and 54 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more