Db and errors #341

ncalexan · 2017-02-22T18:43:30Z

This commit sequence is a bit all over the place. It's all preparatory work for a top-level Conn, as tracked by #296. That's still in progress in my tree.

The first series of commits, up to and including ncalexan@201ffc7, are about separating the SQLite bits from the DB type. See the comments around the MentatStoring trait.

The last few commits, including and following ncalexan@b8a7c84, are about moving the existing code to error-chain. You can see the chaining (a little), and unifying some error types. This also surfaces errors out of combine; see some of the commit messages for notes.

ncalexan · 2017-02-22T18:44:16Z

@rnewman lots of code movement in the DB layer, and then some real work to use error-chain throughout. Hope it makes some sense.

rnewman · 2017-02-23T04:55:51Z

db/src/db.rs

-                                         .chain(once(&tx as &ToSql)
-                                                .chain(once(to_bool_ref(added) as &ToSql)
-                                                       .chain(once(flags as &ToSql)))))))
+                                         .chain(once(to_bool_ref(added) as &ToSql)


Really need some macrology here! Alas, it's beyond my current skills.

I concur: #261.

rnewman · 2017-02-23T05:04:23Z

db/src/debug.rs

-            e: to_entid(schema, e),
-            a: to_entid(schema, a),
+            e: to_entid(schema.borrow(), e),
+            a: to_entid(schema.borrow(), a),


Does it make a difference if we borrow() only once?

rnewman · 2017-02-23T05:05:47Z

db/src/debug.rs

-            e: to_entid(db, e),
-            a: to_entid(db, a),
+            e: to_entid(schema.borrow(), e),
+            a: to_entid(schema.borrow(), a),


Does it make a difference if we only borrow once here?

Nope, so I've lifted a temporary.

rnewman · 2017-02-23T05:08:55Z

db/src/db.rs

+    ///
+    /// Use this to create temporary tables, prepare indices, set pragmas, etc, before the initial
+    /// `insert_non_fts_searches` invocation.
+    fn begin_transaction(&self) -> Result<()>;


This is a difficult thing to abstract, but yeah, I guess!

Yeah, I'm basically just punting and working in a live scratchpad until the abstractions get more clear.

rnewman · 2017-02-23T05:11:26Z

parser-utils/src/lib.rs

+    fn from(e: combine::primitives::ParseError<&'a [edn::Value]>) -> ValueParseError {
+        ValueParseError {
+            position: e.position,
+            errors: e.errors.into_iter().map(|e| e.map_range(|r| {


I confess that I'm a little surprised to have errors be something we can move, but I guess that helps!

I was worried that we'd have copying on the error path, where the error might be due to OOM…

I can only imagine this does badly in OOM situations, but we're not going to get combine::ParseError instances in those situations anyway. I guess it's possible we get ParseError into OOM... but that should blow up during this mapping.

rnewman · 2017-02-23T05:16:13Z

query-parser/src/parse.rs

 }

 #[allow(dead_code)]
 pub fn clause_seq_to_patterns(clauses: &[edn::Value]) -> WhereParseResult {
    Where::clauses()
        .parse(clauses)
        .map(|x| x.0)
-        .map_err(|_| WhereParseError::Err)
+        .map_err::<ValueParseError, _>(|e| e.translate_position(clauses).into())


This fixes #328.

Will close it when I push.

rnewman · 2017-02-23T05:24:10Z

This is fine. I kinda dislike error_chain; it's obtuse, but so it goes.

rnewman · 2017-02-23T05:24:54Z

Err… now I can't mark this as approved. Well done, GitHub.

The idea is to separate the transaction applying code, which is schema-aware, from the concrete storage code, which is just concerned with getting bits onto disk.

This is part of a larger separation of the volatile PartitionMap, which is modified every transaction, from the stable Schema, which is infrequently modified.

This makes it easy to use Rc<Schema> or Arc<Schema> without inserting &* sigils throughout the code.

This fails right now, because we allocate tx IDs even when we shouldn't.

This can be simplified when Marwes/combine#86 makes it to a published release, but this unblocks us for now. This converts the `combine` error type `ParseError<&'a [edn::Value]>` to a type with owned `Vec<edn::Value>` collections, re-using `edn::Value::Vector` for making them `Display`.

There are a few things to point out here: - the fine grained error types have been flattened into one crate-wide error type; it's pretty easy to regain the granularity as needed. - edn::ParseError is automatically lifted to mentat_query_parser::errors::Error; - we use mentat_parser_utils::ValueParser to maintain parsing error information from `combine`.

…#328. r=rnewman (#341) * Pre: Drop unneeded tx0 from search results. * Pre: Don't require a schema in some of the DB code. The idea is to separate the transaction applying code, which is schema-aware, from the concrete storage code, which is just concerned with getting bits onto disk. * Pre: Only reference Schema, not DB, in debug module. This is part of a larger separation of the volatile PartitionMap, which is modified every transaction, from the stable Schema, which is infrequently modified. * Pre: Fix indentation. * Extract part of DB to new SchemaTypeChecking trait. * Extract part of DB to new PartitionMapping trait. * Pre: Don't expect :db.part/tx partition to advance when tx fails. This fails right now, because we allocate tx IDs even when we shouldn't. * Sketch a db interface without DB. * Add ValueParseError; use error-chain in tx-parser. This can be simplified when Marwes/combine#86 makes it to a published release, but this unblocks us for now. This converts the `combine` error type `ParseError<&'a [edn::Value]>` to a type with owned `Vec<edn::Value>` collections, re-using `edn::Value::Vector` for making them `Display`. * Pre: Accept Borrow<Schema> instead of just &Schema in debug module. This makes it easy to use Rc<Schema> or Arc<Schema> without inserting &* sigils throughout the code. * Use error-chain in query-parser. There are a few things to point out here: - the fine grained error types have been flattened into one crate-wide error type; it's pretty easy to regain the granularity as needed. - edn::ParseError is automatically lifted to mentat_query_parser::errors::Error; - we use mentat_parser_utils::ValueParser to maintain parsing error information from `combine`. * Patch up top-level. * Review comment: Only `borrow()` once.

ncalexan · 2017-02-24T23:34:26Z

Gah, force pushing a replacement since I made two typoes in the commit message.

jaredhirsch added the in progress label Feb 22, 2017

ncalexan requested a review from rnewman February 22, 2017 18:43

This was referenced Feb 22, 2017

Add top-level Conn. Fixes #296. #342

Merged

[core] Use combine 2.3.0 to simplify ValueParseError #343

Closed

Moar SQL #339

Closed

rnewman reviewed Feb 23, 2017

View reviewed changes

ncalexan added 12 commits February 24, 2017 11:36

Pre: Fix indentation.

2b567ca

Pre: Drop unneeded tx0 from search results.

a93e357

Pre: Don't require a schema in some of the DB code.

d92a550

The idea is to separate the transaction applying code, which is schema-aware, from the concrete storage code, which is just concerned with getting bits onto disk.

Pre: Only reference Schema, not DB, in debug module.

18c09b0

This is part of a larger separation of the volatile PartitionMap, which is modified every transaction, from the stable Schema, which is infrequently modified.

Pre: Accept Borrow<Schema> instead of just &Schema in debug module.

a04e909

This makes it easy to use Rc<Schema> or Arc<Schema> without inserting &* sigils throughout the code.

Pre: Don't expect :db.part/tx partition to advance when tx fails.

e2e9d8a

This fails right now, because we allocate tx IDs even when we shouldn't.

Extract part of DB to new SchemaTypeChecking trait.

9d60a8d

Extract part of DB to new PartitionMapping trait.

a682464

Sketch a db interface without DB.

f133c50

Patch up top-level.

50afa62

ncalexan force-pushed the db-and-errors branch from a2a39c6 to 5ddf832 Compare February 24, 2017 19:40

Review comment: Only borrow() once.

3d3328c

ncalexan force-pushed the db-and-errors branch from 5ddf832 to 3d3328c Compare February 24, 2017 23:31

ncalexan merged commit ac724bb into mozilla:rust Feb 24, 2017

jaredhirsch removed the in progress label Feb 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Db and errors #341

Db and errors #341

ncalexan commented Feb 22, 2017

ncalexan commented Feb 22, 2017

rnewman Feb 23, 2017

ncalexan Feb 24, 2017

rnewman Feb 23, 2017

rnewman Feb 23, 2017

ncalexan Feb 24, 2017

rnewman Feb 23, 2017

ncalexan Feb 24, 2017

rnewman Feb 23, 2017

ncalexan Feb 24, 2017

rnewman Feb 23, 2017

ncalexan Feb 24, 2017

rnewman commented Feb 23, 2017

rnewman commented Feb 23, 2017

ncalexan commented Feb 24, 2017

Db and errors #341

Db and errors #341

Conversation

ncalexan commented Feb 22, 2017

ncalexan commented Feb 22, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnewman commented Feb 23, 2017

rnewman commented Feb 23, 2017

ncalexan commented Feb 24, 2017