fix: Hybrid planning when the root plan has local preference #2183

vrongmeal · 2023-11-30T09:42:07Z

Push down "remote" exec as far down as possible and replace any "local"
execs that are found (children to the remote). Finally, pack the
remote exec in a [SendRecvJoinExec] so the plan transformation looks
something like:

Original plan:

                 +-----------+
                 | A (Local) |
                 +-----+-----+
                       |
                       v
              +-----------------+
      +-------+ B (Unspecified) +-------+
      |       +--------+--------+       |
      v                |                v
+------------+         |          +-----------+
| C (Remote) |         |          | F (Local) |
+-----+------+         v          +-----------+
      |          +------------+
      v          | E (Remote) |
+-----------+    +------------+
| D (Local) |
+-----------+

Transformed plan:

                          +-----------+
                          | A (Local) |
                          +-----+-----+
                                |
                                v
                       +-----------------+
         +-------------+ B (Unspecified) +-------+
         |             +--------+--------+       |
         |                      |                v
         v                      |          +-----------+
+------------------+            |          | F (Local) |
| SendRecvJoinExec |            v          +-----------+
|                  |   +------------------+
|        C         |   | SendRecvJoinExec |
|        |         |   |                  |
|        v         |   |        E         |
|   D (Send-Recv)  |   +------------------+
+------------------+

Note: All nodes shown in the graphs above (except B) are
[RuntimeGroupExec]s. Other nodes are omitted for simplicity.

We don't "pull-up" the remote exec as much as possible because we want
to stop pulling once a "local" node is met but there's no certain way of
knowing that the node is to be run locally (since that information lies
with its n'th parent, i.e., [RuntimeGroupExec]).

For example, take the following plan in consideration which tries to
copy contents from a remote table in a local file:

RuntimeExec (local)
        |
    CopyToExec
        |
      Limit
        |
RuntimeExec (remote)
        |
    TableScan

If we were to pull "remote" up until the "local", we would end up with
something like:

RuntimeExec (local)
        |
RuntimeExec (remote)
        |
    CopyToExec
        |
      Limit
        |
    TableScan

This ends up running the CopyToExec on remote which is completely
incorrect.

Fixes: #2104

tychoish · 2023-11-30T13:43:01Z

crates/datafusion_ext/src/vars.rs

@@ -95,6 +95,7 @@ impl SessionVars {
     is_cloud_instance: bool,
     dialect: Dialect,
     enable_experimental_scheduler: bool,
+     prefer_local_execution: bool,


is this really "prefer" and not "force"

are there meaningfully cases in local/embedded nodes where there's more-than-one session? My instinct was that local sessions (which are the things where hybrid execution could have happened anyway,) were always single user, and had one copy of the glaredb instance loaded/running.

seems like we should (maybe) have this be an option on the operation itself rather than on the session (which means that the state of "should this operation be remote or local" is stateful on the session, and could be weird in multi-threaded contexts.

prefer_local_execution might not be the best name for this, but it's really more about setting the default location of where to run parts of the query. Currently we default to "remote" at the root and selectively pull up "local" parts as much as possible. This variable really just changes it to default "local" at the root.

I suggested a session variable since changing the default could break something that we don't know about, and so having a way to go back to the other behavior seemed reasonable and lets us play with it.

can we just have this be "execution_mode" and have it be an enum?

should_hybrid_execution_default_local

if it defaults to local at the root will any parts be remote?

can we just have this be "execution_mode" and have it be an enum?

We could, but I don't know if it's worth doing here. What we're aiming to solve here is very specific to hybrid exec, which only really cares about is thing running locally, or is thing not running locally. It won't (or shouldn't) care about how the thing is running remotely once we have dist exec.

should_hybrid_execution_default_local

Seems reasonable to me. I don't see this being a variable that a user really cares about unless they hit an edge case that would require them to set this, so the verbosity makes sense.

if it defaults to local at the root will any parts be remote?

Yes, runtime group effect everything below it in the tree unless there's another runtime group.

I will do my best to draw a picture (assuming local at root):

RuntimeGroup (local) | Project | Join | -------------- | | RuntimeGroup (remote) RuntimeGroup (remote) | | TableScan TableScan

What this is saying is both table scans should run remotely, the results of both be pulled down locally, joined locally, and projected locally.

We have an optimizer rule that will pull up runtime groups as far as possible to minimize how much we need to pull down locally. After the rule, the tree ends up being:

RuntimeGroup (remote) | Project | Join | -------------- | | TableScan TableScan

The table scans, join, and project now all run remotely, and the results of the entire tree are then returned to the local client.

I am working on the same, as Sean mentioned. Right now, the "remote" isn't pulled up; it must also be configurable. I will mark it ready for review once it is done.

[sounds good to me! sorry if I complicated things, but this feels clearer!]

I am working on the same, as Sean mentioned. Right now, the "remote" isn't pulled up; it must also be configurable. I will mark it ready for review once it is done.

Oh, I assumed it was already doing that. If we can have it pull up matching preferences, that would be ideal (and would be useful once extended for dist exec too).

crates/sqlexec/src/remote/planner.rs

scsmithr · 2023-11-30T18:19:31Z

crates/datafusion_ext/src/vars.rs

@@ -95,6 +95,7 @@ impl SessionVars {
     is_cloud_instance: bool,
     dialect: Dialect,
     enable_experimental_scheduler: bool,
+     prefer_local_execution: bool,


can we just have this be "execution_mode" and have it be an enum?

We could, but I don't know if it's worth doing here. What we're aiming to solve here is very specific to hybrid exec, which only really cares about is thing running locally, or is thing not running locally. It won't (or shouldn't) care about how the thing is running remotely once we have dist exec.

should_hybrid_execution_default_local

Seems reasonable to me. I don't see this being a variable that a user really cares about unless they hit an edge case that would require them to set this, so the verbosity makes sense.

if it defaults to local at the root will any parts be remote?

Yes, runtime group effect everything below it in the tree unless there's another runtime group.

I will do my best to draw a picture (assuming local at root):

RuntimeGroup (local) | Project | Join | -------------- | | RuntimeGroup (remote) RuntimeGroup (remote) | | TableScan TableScan

What this is saying is both table scans should run remotely, the results of both be pulled down locally, joined locally, and projected locally.

We have an optimizer rule that will pull up runtime groups as far as possible to minimize how much we need to pull down locally. After the rule, the tree ends up being:

RuntimeGroup (remote) | Project | Join | -------------- | | TableScan TableScan

The table scans, join, and project now all run remotely, and the results of the entire tree are then returned to the local client.

vrongmeal · 2023-12-04T13:45:35Z

NOTE: Trying to fix copy-to. There's still some issue with it being executed on remote.

EDIT: The optimization is causing some issues. Thinking about it a bit more.

scsmithr

I think this makes sense.

[TODO]: Need to add more comments.

Yes, we definitely need to. I think we'll also want to have tests asserting pre and post plan transformations, which would make this stuff a bit clearer too.

Wrapping custom execs in the RuntimeGroupExec seems to make sense here, and makes it a bit clearer on expectations for where something is ran.

Fixes: #2104 Signed-off-by: Vaibhav <vrongmeal@gmail.com>

scsmithr · 2023-12-05T15:20:35Z

crates/sqlexec/src/remote/planner.rs

+    /// incorrect.
+    // TODO: Figure out how to "pull-up" remote more accurately. One solution
+    // might be to specialize cases where we know we can pull up, i.e., know
+    // that the node has to execute on local.


I really appreciate this comment.

tychoish reviewed Nov 30, 2023

View reviewed changes

scsmithr reviewed Nov 30, 2023

View reviewed changes

vrongmeal force-pushed the vrongmeal/prefer-local-execution branch 2 times, most recently from d0c0269 to 73dad61 Compare December 4, 2023 12:29

vrongmeal changed the title ~~feat: Add option to prefer local execution over remote~~ fix: Hybrid planning when the root plan has local preference Dec 4, 2023

vrongmeal requested review from scsmithr and universalmind303 December 4, 2023 12:29

vrongmeal marked this pull request as ready for review December 4, 2023 12:29

vrongmeal marked this pull request as draft December 4, 2023 13:45

scsmithr approved these changes Dec 4, 2023

View reviewed changes

vrongmeal added 2 commits December 5, 2023 17:36

fix: Hybrid planning when the root plan has local preference

baecacb

Fixes: #2104 Signed-off-by: Vaibhav <vrongmeal@gmail.com>

fixup! fix: Hybrid planning when the root plan has local preference

f699c40

vrongmeal force-pushed the vrongmeal/prefer-local-execution branch from 73dad61 to f699c40 Compare December 5, 2023 12:06

vrongmeal marked this pull request as ready for review December 5, 2023 12:32

vrongmeal requested a review from scsmithr December 5, 2023 12:39

scsmithr approved these changes Dec 5, 2023

View reviewed changes

Merge branch 'main' into vrongmeal/prefer-local-execution

bd8e6be

vrongmeal enabled auto-merge (squash) December 5, 2023 17:24

vrongmeal merged commit ef8f164 into main Dec 5, 2023
10 checks passed

vrongmeal deleted the vrongmeal/prefer-local-execution branch December 5, 2023 17:48

vrongmeal mentioned this pull request Dec 6, 2023

COPY TO(remote): External error: Execution error: Cannot execute remote scan with a remote reference #2198

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Hybrid planning when the root plan has local preference #2183

fix: Hybrid planning when the root plan has local preference #2183

vrongmeal commented Nov 30, 2023 •

edited

tychoish Nov 30, 2023 •

edited

scsmithr Nov 30, 2023

tychoish Nov 30, 2023

scsmithr Nov 30, 2023

vrongmeal Dec 1, 2023

tychoish Dec 1, 2023

scsmithr Dec 1, 2023

scsmithr Nov 30, 2023

vrongmeal commented Dec 4, 2023 •

edited

scsmithr left a comment

scsmithr Dec 5, 2023

fix: Hybrid planning when the root plan has local preference #2183

fix: Hybrid planning when the root plan has local preference #2183

Conversation

vrongmeal commented Nov 30, 2023 • edited

Original plan:

Transformed plan:

tychoish Nov 30, 2023 • edited

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

vrongmeal commented Dec 4, 2023 • edited

scsmithr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrongmeal commented Nov 30, 2023 •

edited

tychoish Nov 30, 2023 •

edited

vrongmeal commented Dec 4, 2023 •

edited