Finish passing input parameters through to queries by rnewman · Pull Request #418 · mozilla/mentat

rnewman · 2017-04-17T22:08:48Z

In the course of starting another feature, I realized this wasn't finished.

rnewman · 2017-04-17T23:15:05Z

I just realized this uses inputs not named in :in. Fixing.

rnewman · 2017-04-17T23:49:38Z

Oh, and we don't parse :in yet. Heh.

This also adds a test that an `UnboundVariables` error is raised if a variable mentioned in the `:in` clause isn't bound.

rnewman · 2017-04-18T04:33:28Z

@ncalexan: part 1 needs some advice from you on how to get things to compile when returning an error; I can't get it to work.

rnewman · 2017-04-18T14:46:56Z

core/src/lib.rs

-            &ValueType::Double => values::DB_TYPE_DOUBLE.clone(),
-            &ValueType::String => values::DB_TYPE_STRING.clone(),
-            &ValueType::Keyword => values::DB_TYPE_KEYWORD.clone(),
+        match *self {


I did this on the way, but I suspect it should actually be to_edn_value(self) — ValueType is Copy, so avoiding references wherever possible is best.

rnewman · 2017-04-18T14:47:39Z

query-algebrizer/src/clauses/mod.rs

+            None => ConjoiningClauses::with_alias_counter(alias_counter),
+            Some(QueryInputs { mut types, mut values }) => {
+                println!("Got inputs {:?}", values);
+                println!("Got input vars {:?}", in_variables);


I'll remove these locally.

We also at this point switch from using `Vec<Variable>` to `BTreeSet<Variable>`. This allows us to guarantee no duplicates later; we'll reject duplicates at parse time.

This is for two reasons. Firstly, we need to track the types of inputs, their values, and also the input variables; adding a struct gives us a little more clarity. Secondly, when we come to implement prepared statements, we'll be algebrizing queries without having the values available. We'll be able to do a better job of algebrizing, and also do more validating, if we allow callers to specify the types of variables in advance, even if the values aren't known.

We'll use this to drop unneeded values from input maps, if lazy callers reuse a general-purpose map for multiple queries.

This also adds a test that an `UnboundVariables` error is raised if a variable mentioned in the `:in` clause isn't bound.

…; use combine::primitive::Error.

ncalexan

Few nits, some small requests. Rock on!

ncalexan · 2017-04-18T20:04:10Z

query-parser/src/parse.rs

    }

    errors {
+        DuplicateVariableError {


Worth decorating with the variable, I think. (I see your TODO below.)

Unfortunately it's a little tricky with the way I've implemented it; I hope this can be part of an ergonomics pass in a few months. It should be pretty obvious to users when there are duplicated variables.

ncalexan · 2017-04-18T20:04:35Z

query/src/lib.rs

 pub struct Order(pub Direction, pub Variable);   // Future: Element instead of Variable?

-#[derive(Clone, Debug, Eq, PartialEq)]
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]


nit: prefer alphabetical order.

This was copypasta'd from elsewhere, but sure.

ncalexan · 2017-04-18T20:06:29Z

query-algebrizer/src/clauses/inputs.rs

+    Variable,
+};
+
+pub use errors::{


Oh, I get your question. Yeah, probably unnecessary.

ncalexan · 2017-04-18T20:07:08Z

query-algebrizer/src/clauses/inputs.rs

+/// When built correctly, `types` is guaranteed to contain the types of `values` -- use
+/// `QueryInputs::new` or `QueryInputs::with_values` to construct an instance.
+pub struct QueryInputs {
+    pub types: BTreeMap<Variable, ValueType>,


Do these need to be pub? You could make a stronger statement if they were hidden, and only exposed via accessors.

I'd like them not to be, but there's no concept of crate-private in Rust. I could flatten inputs.rs into mod.rs…

ncalexan · 2017-04-18T20:07:47Z

query-algebrizer/src/clauses/inputs.rs

+
+impl Default for QueryInputs {
+    fn default() -> Self {
+        QueryInputs { types: BTreeMap::default(), values: BTreeMap::default() }


Didn't we settle on

Name { x: X, y: Y, ... }

(Here and below.)

ncalexan · 2017-04-18T20:14:52Z

query-algebrizer/src/clauses/mod.rs

+    /// Remove all keys from the map that are not present in `ks`.
+    /// This implementation is terrible because there's no mutable iterator for BTreeMap.
+    fn keep_intersected_keys(&mut self, ks: &BTreeSet<K>) {
+        let mut to_remove = Vec::with_capacity(self.len() - ks.len());


I expect this to panic when self.len() is less than ks.len(). That might be okay, but can you be safe about it?

Good catch!

ncalexan · 2017-04-18T20:15:53Z

query-algebrizer/src/clauses/mod.rs


 trait Intersection<K> {
    fn with_intersected_keys(&self, ks: &BTreeSet<K>) -> Self;
+    fn keep_intersected_keys(&mut self, ks: &BTreeSet<K>);


You have one mutating and one non-mutating method. Is there a compelling reason to have different API regimes?

Well, it means we can write:

value_bindings: self.value_bindings.with_intersected_keys(&vars),

rather than

let new_value_bindings = self.value_bindings.clone(); new_value_bindings.keep_intersected_keys(&vars); … value_bindings: new_value_bindings,

The reason for k_i_k is because we're already taking ownership of the inputs, so it's wasteful to clone into a new map.

ncalexan · 2017-04-18T20:16:57Z

query-algebrizer/src/clauses/mod.rs

+        match inputs.into() {
+            None => ConjoiningClauses::with_alias_counter(alias_counter),
+            Some(QueryInputs { mut types, mut values }) => {
+                // Discard any bindings not mentioned in our :in clause.


Do we test somewhere that all :in variables appear? I feel like Datomic probably doesn't care, but we should probably fail in this case, since it's likely to be an error.

We do check that they're all bound. I don't think we check that they're a subset of the variables in the query. I'll file.

ncalexan · 2017-04-18T20:18:50Z

src/errors.rs


    errors {
+        UnboundVariables(names: BTreeSet<String>) {
+            description("unbound variables at execution time")


nit: "query execution time"? There are many things being executed at many times.

ncalexan · 2017-04-18T20:19:38Z

src/query.rs

+    // Because this is q_once, we can check that all of our `:in` variables are bound at this point.
+    // If they aren't, the user has made an error -- perhaps writing the wrong variable in `:in`, or
+    // not binding in the `QueryInput`.
+    let unbound = algebrized.unbound_variables();


Ah! I think this closes an open loop from earlier.

I did this because I wrote a test that failed because I typoed the :in variable :)

This also adds a test that an `UnboundVariables` error is raised if a variable mentioned in the `:in` clause isn't bound.

This commit downgrades error_chain to 0.8.1 in order to fix trait bounds on errors.

rnewman · 2017-04-18T20:21:28Z

Thanks for the help, Nick!

aa14a71

rnewman · 2017-04-18T20:51:25Z

Review comments addressed in bffefe7.

rnewman added 2 commits April 17, 2017 14:19

Pre: unused imports.

fbf4299

Pre: add two checks to translate tests to fix unused var warning.

3df857a

rnewman self-assigned this Apr 17, 2017

rnewman requested a review from ncalexan April 17, 2017 22:08

rnewman added the in progress label Apr 17, 2017

rnewman force-pushed the rnewman/inline-limit branch from eb5a529 to 15d0f5d Compare April 17, 2017 22:09

rnewman added a commit that referenced this pull request Apr 17, 2017

Pass inputs into algebrizing and execution. (#418)

15d0f5d

rnewman force-pushed the rnewman/inline-limit branch from 15d0f5d to 4955156 Compare April 17, 2017 22:10

rnewman added a commit that referenced this pull request Apr 17, 2017

Pass inputs into algebrizing and execution. (#418)

4955156

Pre: take Variables instead of Strings in public API, for now.

b614632

rnewman force-pushed the rnewman/inline-limit branch from 4955156 to 3297774 Compare April 18, 2017 04:32

rnewman added a commit that referenced this pull request Apr 18, 2017

Part 4: pass inputs through algebrizing and execution. (#418)

3297774

This also adds a test that an `UnboundVariables` error is raised if a variable mentioned in the `:in` clause isn't bound.

rnewman commented Apr 18, 2017

View reviewed changes

rnewman added 5 commits April 18, 2017 07:53

Pre: define Display for ValueType.

0e3a787

Part 1: parse :in.

17db8e4

We also at this point switch from using `Vec<Variable>` to `BTreeSet<Variable>`. This allows us to guarantee no duplicates later; we'll reject duplicates at parse time.

Part 3: define keep_intersected_keys.

d7e0425

We'll use this to drop unneeded values from input maps, if lazy callers reuse a general-purpose map for multiple queries.

Part 4: pass inputs through algebrizing and execution. (#418)

dbbfdb2

This also adds a test that an `UnboundVariables` error is raised if a variable mentioned in the `:in` clause isn't bound.

rnewman force-pushed the rnewman/inline-limit branch from 3297774 to dbbfdb2 Compare April 18, 2017 14:53

Review comments: downgrade to error-chain 0.8.1 for Send + Sync bound…

8a29f86

…; use combine::primitive::Error.

rnewman mentioned this pull request Apr 18, 2017

Add a Sync bounds to errors rust-lang-deprecated/error-chain#110

Merged

ncalexan approved these changes Apr 18, 2017

View reviewed changes

rnewman added a commit that referenced this pull request Apr 18, 2017

Part 4: pass inputs through algebrizing and execution. (#418)

60c082b

This also adds a test that an `UnboundVariables` error is raised if a variable mentioned in the `:in` clause isn't bound.

rnewman added a commit that referenced this pull request Apr 18, 2017

Parse :in, pass inputs through to querying. (#418) r=nalexander

aa14a71

This commit downgrades error_chain to 0.8.1 in order to fix trait bounds on errors.

rnewman closed this Apr 18, 2017

rnewman removed the in progress label Apr 18, 2017

rnewman deleted the rnewman/inline-limit branch April 18, 2017 20:21

rnewman added a commit that referenced this pull request Apr 18, 2017

Review comments for #418.

bffefe7

rnewman mentioned this pull request Mar 5, 2018

[query] Warn or error when :in variables are supplied that aren't used in the query #421

Closed

Conversation

rnewman commented Apr 17, 2017

Uh oh!

rnewman commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnewman commented Apr 17, 2017

Uh oh!

rnewman commented Apr 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncalexan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnewman commented Apr 18, 2017

Uh oh!

rnewman commented Apr 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rnewman commented Apr 17, 2017 •

edited

Loading