RFC: Opaque context in expression evaluation #75
Conversation
Signed-off-by: TennyZhuang <[email protected]>
Signed-off-by: TennyZhuang <[email protected]>
|
|
||
| ### task_local | ||
|
|
||
| tokio's [`task_local` macro](https://docs.rs/tokio/latest/tokio/macro.task_local.html) can achieve the same result even simpler, and the only cost is the extensive use of global variables. |
There was a problem hiding this comment.
task_local is not that "global" as the calling stack or the scope of evaluating an expression tree is quite clear. I would vote for this approach as it introduces minimal changes to the codebase.
There was a problem hiding this comment.
+1 for this approach. Adding context to the eval interface will make things more complicated and prevent easy use for most of the pure functions. Every time we call eval, we have to find out a context to pass in...
It's annoying to let developers pass context everywhere themselves. I once tried that in madsim, but soon gave it up and made the context global.
There was a problem hiding this comment.
You can even pass a () as context here. I don’t think it’s a heavy problem.
The key concept is pass it explicitly.
There was a problem hiding this comment.
We can also add a eval_with_empty_context.
It’s really simple.
There was a problem hiding this comment.
In fact, all eval needs a context and I'd prefer pass it explicitly instead of the one from an unknown scope.
There was a problem hiding this comment.
Another solution: is it possible to put the context into the expressions themselves? I've discussed it with @shanicky yesterday.
Not good. "The lifetime of the context will be longer".
Yes, my point is explicit context will put more burden on developers and make it error-prone
I think evaluating expression doesn't happen everywhere. For example, in backend, it should only happen in a few executors: Project and Join (for inner join condition). Passing an explicit context helps to make it less error pruning.
On the other hand, if a developer does need to eval in some strange place, get_context will return None and the evaluation should act in default behavior (such as with UTC+0 timezone). The missing context (like Context::empty()) always warns him about this behavior, as long as he takes the risk, he can do it.
There was a problem hiding this comment.
Besides, from my understanding, @TennyZhuang's point to introduce provide instead of using a simple struct ExprContext is he wants to make it more flexible. For example, if BackendContext exists, then the eval() function can get it; otherwise, it gets None. So if you are sure that no context is needed somewhere (I don't know what can be the case... but just saying), it's possible to use an empty context.
There was a problem hiding this comment.
Yes, but we need to move the expression construction out of the expr crate.
Not necessary? We can pass the opaque context into the constructor function.
There was a problem hiding this comment.
I think evaluating expression doesn't happen everywhere.
It does happen everywhere in unit tests. And we have to provide an empty context for each of them. However, I think providing a eval_with_default_context method is acceptable.
There was a problem hiding this comment.
It does happen everywhere in unit tests.
Well. Indeed. 😄
If it costs too many changes to replace eval to eval_with_default_context, scoped task_local might look a little bit better. Either way LGTM.
By the way, it's worth mentioning that using provide or not is orthogonal to whether to put context in function call arguments or task_local: you may also put the Context: Provider as a task-local variable.
fuyufjh
changed the title
Rendered