Use Rc for TypedValue and Variable. (#395) r=nalexander#397
Conversation
rnewman
left a comment
rnewman
left a comment
There was a problem hiding this comment.
There is a surprisingly small amount of non-test code here.
We shall see how this looks with a little interning.
@ncalexan, let me know in the morning if you think you'll get your parser stuff landed sooner rather than later; there's a little bitrotting, and I suspect it'll be more painful for you.
| (5, rusqlite::types::Value::Integer(x)) => Ok(TypedValue::Long(x)), | ||
| (5, rusqlite::types::Value::Real(x)) => Ok(TypedValue::Double(x.into())), | ||
| (10, rusqlite::types::Value::Text(x)) => Ok(TypedValue::String(x)), | ||
| (10, rusqlite::types::Value::Text(x)) => Ok(TypedValue::String(Rc::new(x))), |
There was a problem hiding this comment.
I wonder if from_sql_value_pair should take an InternSet<String>…
There was a problem hiding this comment.
Or, more generally, a transforming function.
| &Value::Float(ref x) => Some(TypedValue::Double(x.clone())), | ||
| &Value::Text(ref x) => Some(TypedValue::String(x.clone())), | ||
| &Value::NamespacedKeyword(ref x) => Some(TypedValue::Keyword(x.clone())), | ||
| &Value::Text(ref x) => Some(TypedValue::String(Rc::new(x.clone()))), |
| &TypedValue::Double(x) => (Value::Float(x), ValueType::Double), | ||
| &TypedValue::String(ref x) => (Value::Text(x.clone()), ValueType::String), | ||
| &TypedValue::Keyword(ref x) => (Value::NamespacedKeyword(x.clone()), ValueType::Keyword), | ||
| &TypedValue::String(ref x) => (Value::Text(x.as_ref().clone()), ValueType::String), |
There was a problem hiding this comment.
If we make edn::Value use Rc, we can avoid ballooning the stream — strings would be pseudo-unique.
| } | ||
| if let TypedValue::Keyword(keyword) = typed_value { | ||
| Ok((keyword, e)) | ||
| Ok((keyword.as_ref().clone(), e)) |
There was a problem hiding this comment.
We know that the keys of the map are unique, so there's no value in having IdentMap use Rc.
| } else { | ||
| // It must be a keyword. | ||
| self.constrain_column_to_constant(col.clone(), DatomsColumn::Value, TypedValue::Keyword(kw.clone())); | ||
| self.constrain_column_to_constant(col.clone(), DatomsColumn::Value, TypedValue::Keyword(Rc::new(kw.clone()))); |
There was a problem hiding this comment.
Interning needs to be added here, or in the query parser itself.
Part 1, core: use Rc for String and Keyword. Part 2, query: use Rc for Variable. Part 3, sql: use Rc for args in SQLiteQueryBuilder. Part 4, query-algebrizer: use Rc. Part 5, db: use Rc. Part 6, query-parser: use Rc. Part 7, query-projector: use Rc. Part 8, query-translator: use Rc. Part 9, top level: use Rc.
|
Slipped in interning of |
1 participant
Part 1, core: use Rc for String and Keyword.
Part 2, query: use Rc for Variable.
Part 3, sql: use Rc for args in SQLiteQueryBuilder.
Part 4, query-algebrizer: use Rc.
Part 5, db: use Rc.
Part 6, query-parser: use Rc.
Part 7, query-projector: use Rc.
Part 8, query-translator: use Rc.
Part 9, top level: use Rc.