rust: use Rc<RuntimeValue> to avoid deep copies#212
Conversation
✅ Deploy Preview for mistql canceled.
|
dshemetov
changed the title
Rc<RuntimeValue> to make a single json copyRc<RuntimeValue> to avoid deep copies
evinism
left a comment
evinism
left a comment
There was a problem hiding this comment.
Looks pretty good, thanks for doing this! Only thing I really care about is the Rc<> on the external interface, everything else looks gtg.
| { | ||
| let runtime_data = data.try_into().map_err(|e| MistQLError::Runtime(e.to_string()))?; | ||
| query_runtime(query_str, &runtime_data) | ||
| pub fn query(query_str: &str, data: &Value) -> Result<Rc<RuntimeValue>, MistQLError> { |
There was a problem hiding this comment.
Returning an RC runtime value feels like an implementation leak. We should return a bare value, and just use RC as an internal optimization rather than expose it to the interface.
|
|
||
| // AST expression types for MistQL | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub enum LiteralValue { |
There was a problem hiding this comment.
NB: Could call this LiteralPrimitive, since technically [foo, bar] is considered an array literal.
| Regex(MistQLRegex), | ||
| } | ||
|
|
||
| impl RuntimeValue { |
There was a problem hiding this comment.
NB: Could also likely replace these constructor functions with pub fn rc(self) -> Rc<Self> such that we can do RuntimeValue::Boolean(true).rc(). Might be slightly clearer / more rust-y / not pollute the returned RuntimeValue struct with as many constructors.
2 participants
After a month of tinkering with overly clever lazy evaluation approaches and get tangled in too many lifetimes, I arrived at an unoriginal "Rc all the things!" solution, but hey, at least it's clean and does the job.