Dandandan commented on code in PR #24326:
URL: https://github.com/apache/datafusion/pull/24326#discussion_r3776690194
##########
datafusion/session/src/table.rs:
##########
@@ -338,46 +340,84 @@ pub trait TableProvider: Any + Debug + Sync + Send {
/// streams of `RecordBatch`es as files to an ObjectStore.
///
/// [`DataSinkExec`]:
https://docs.rs/datafusion-datasource/latest/datafusion_datasource/sink/struct.DataSinkExec.html
- async fn insert_into(
- &self,
- _state: &dyn Session,
+ // Compile-time optimization: these defaults are written as the desugaring
of
+ // `async fn` so they return a ready future instead of a coroutine
capturing
+ // `Expr`, whose `Send` proof pulls in the whole `LogicalPlan` graph and is
+ // re-proved per method (~2x faster to compile this crate).
+ fn insert_into<'life0, 'life1, 'async_trait>(
Review Comment:
Good catch, that comment was wrong where it sat — `insert_into` captures
`&dyn Session`, `Arc<dyn ExecutionPlan>` and `InsertOp`, no logical types at
all.
It also prompted me to check whether converting it bought anything, and it
did not. Reverting each method one at a time and measuring
`evaluate_obligation` self time (all-converted baseline 659ms):
| method left as `async fn` | trait solving | marginal cost |
|---|---|---|
| `delete_from` | 1.29s | +631ms |
| `update` | 1.30s | +641ms |
| `merge_into` | 1.29s | +631ms |
| `truncate` | 697ms | +38ms |
| `insert_into` | 665ms | ~0 |
Only the three that actually take `Expr` / `Vec<Expr>` matter — proving
their coroutine `Send` walks the whole `Expr`/`LogicalPlan` graph. The two
`planner.rs` bodies measured ~0 as well (652ms with them reverted vs 652ms with
them applied).
So I have cut the PR down to just `delete_from`, `update` and `merge_into`,
reverted `insert_into`, `truncate` and all of `planner.rs`, and rewritten the
comment to describe what is actually captured. Same benefit from a much smaller
diff:
```
base: 1.582s 1.576s 1.590s
fix: 0.933s 0.938s 0.932s
```
Thanks — this made the change considerably smaller.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
