Address review feedback related to translator

core/src/lib.rs

@@ -281,25 +281,33 @@ impl From<i32> for TypedValue {
     }
 }
 
+/// Type safe representation of the possible return values from SQLite's `typeof`
+#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialOrd, PartialEq)]
+pub enum SQLTypeAffinity {
+    Null, // "null"
+    Integer, // "integer"
+    Real, // "real"
+    Text, // "text"
+    Blob, // "blob"
+}
+
 // Put this here rather than in `db` simply because it's widely needed.
 pub trait SQLValueType {
     fn value_type_tag(&self) -> i32;
     fn accommodates_integer(&self, int: i64) -> bool;
+
+    /// Return a pair of the ValueTypeTag for this value type, and the SQLTypeAffinity required
+    /// to distinguish it from any other types that share the same tag.
+    ///
+    /// Background: The tag alone is not enough to determine the type of a value, since multiple
+    /// ValueTypes may share the same tag (for example, ValueType::Long and ValueType::Double).
+    /// However, each ValueType can be determined by checking both the tag and the type's affinity.
+    fn sql_representation(&self) -> (ValueTypeTag, Option<SQLTypeAffinity>);
 }
 
 impl SQLValueType for ValueType {
     fn value_type_tag(&self) -> i32 {
-        match *self {
+        self.sql_representation().0
-            ValueType::Ref =>      0,
-            ValueType::Boolean =>  1,
-            ValueType::Instant =>  4,
-            // SQLite distinguishes integral from decimal types, allowing long and double to share a tag.
-            ValueType::Long =>     5,
-            ValueType::Double =>   5,
-            ValueType::String =>  10,
-            ValueType::Uuid =>    11,
-            ValueType::Keyword => 13,
-        }
     }
 
     /// Returns true if the provided integer is in the SQLite value space of this type. For
@@ -326,6 +334,20 @@ impl SQLValueType for ValueType {
             Uuid                    => false,
         }
     }
+
+    fn sql_representation(&self) -> (ValueTypeTag, Option<SQLTypeAffinity>) {
+        match *self {
+            ValueType::Ref     => (0, None),
+            ValueType::Boolean => (1, None),
+            ValueType::Instant => (4, None),
+            // SQLite distinguishes integral from decimal types, allowing long and double to share a tag.
+            ValueType::Long    => (5, Some(SQLTypeAffinity::Integer)),
+            ValueType::Double  => (5, Some(SQLTypeAffinity::Real)),
+            ValueType::String  => (10, None),
+            ValueType::Uuid    => (11, None),
+            ValueType::Keyword => (13, None),
+        }
+    }
 }
 
 trait EnumSetExtensions<T: enum_set::CLike + Clone> {

query-algebrizer/src/clauses/mod.rs

@@ -934,7 +934,7 @@ impl ConjoiningClauses {
             self.wheres.add_intersection(ColumnConstraint::HasTypes {
                 value: qa.0.clone(),
                 value_types: *types,
-                strict: true,
+                check_value: true,
             });
         }
         if let Some(reason) = empty_because {

query-algebrizer/src/types.rs

@@ -337,7 +337,7 @@ pub enum ColumnConstraint {
     HasTypes {
         value: TableAlias,
         value_types: ValueTypeSet,
-        strict: bool,
+        check_value: bool,
     },
     NotExists(ComputedTable),
     Matches(QualifiedAlias, QueryValue),
@@ -348,7 +348,7 @@ impl ColumnConstraint {
         ColumnConstraint::HasTypes {
             value,
             value_types: ValueTypeSet::of_one(value_type),
-            strict: false
+            check_value: false,
         }
     }
 }
@@ -465,12 +465,12 @@ impl Debug for ColumnConstraint {
                 write!(f, "{:?} MATCHES {:?}", qa, thing)
             },
 
-            &HasTypes { ref value, ref value_types, strict } => {
+            &HasTypes { ref value, ref value_types, check_value } => {
                 // This is cludgey, but it's debug code.
                 write!(f, "(")?;
                 for value_type in value_types.iter() {
                     write!(f, "({:?}.value_type_tag = {:?}", value, value_type)?;
-                    if strict && value_type == ValueType::Double || value_type == ValueType::Long {
+                    if check_value && value_type == ValueType::Double || value_type == ValueType::Long {
                         write!(f, " AND typeof({:?}) = '{:?}')", value,
                                if value_type == ValueType::Double { "real" } else { "integer" })?;
                     } else {

query-sql/src/lib.rs

@@ -19,6 +19,7 @@ use std::boxed::Box;
 use mentat_core::{
     Entid,
     TypedValue,
+    SQLTypeAffinity,
 };
 
 use mentat_query::{
@@ -112,15 +113,6 @@ pub enum Constraint {
     }
 }
 
-/// Type safe representation of the possible return values from `typeof`
-pub enum SQLTypeAffinity {
-    Null, // "null"
-    Integer, // "integer"
-    Real, // "real"
-    Text, // "text"
-    Blob, // "blob"
-}
-
 impl Constraint {
     pub fn not_equal(left: ColumnOrExpression, right: ColumnOrExpression) -> Constraint {
         Constraint::Infix {
@@ -326,19 +318,6 @@ impl QueryFragment for Op {
     }
 }
 
-impl QueryFragment for SQLTypeAffinity {
-    fn push_sql(&self, out: &mut QueryBuilder) -> BuildQueryResult {
-        out.push_sql(match *self {
-            SQLTypeAffinity::Null => "'null'",
-            SQLTypeAffinity::Integer => "'integer'",
-            SQLTypeAffinity::Real => "'real'",
-            SQLTypeAffinity::Text => "'text'",
-            SQLTypeAffinity::Blob => "'blob'",
-        });
-        Ok(())
-    }
-}
-
 impl QueryFragment for Constraint {
     fn push_sql(&self, out: &mut QueryBuilder) -> BuildQueryResult {
         use self::Constraint::*;
@@ -398,7 +377,13 @@ impl QueryFragment for Constraint {
                 out.push_sql("typeof(");
                 value.push_sql(out)?;
                 out.push_sql(") = ");
-                affinity.push_sql(out)?;
+                out.push_sql(match *affinity {
+                    SQLTypeAffinity::Null => "'null'",
+                    SQLTypeAffinity::Integer => "'integer'",
+                    SQLTypeAffinity::Real => "'real'",
+                    SQLTypeAffinity::Text => "'text'",
+                    SQLTypeAffinity::Blob => "'blob'",
+                });
                 Ok(())
             },
         }

query-translator/src/translate.rs

@@ -9,6 +9,7 @@
 // specific language governing permissions and limitations under the License.
 
 use mentat_core::{
+    SQLTypeAffinity,
     SQLValueType,
     TypedValue,
     ValueType,
@@ -51,12 +52,13 @@ use mentat_query_sql::{
     ProjectedColumn,
     Projection,
     SelectQuery,
-    SQLTypeAffinity,
     TableList,
     TableOrSubquery,
     Values,
 };
 
+use std::collections::HashMap;
+
 use super::Result;
 
 trait ToConstraint {
@@ -99,6 +101,51 @@ impl ToConstraint for ColumnConstraintOrAlternation {
     }
 }
 
+fn affinity_count(tag: i32) -> usize {
+    ValueTypeSet::any().into_iter()
+                       .filter(|t| t.value_type_tag() == tag)
+                       .count()
+}
+
+fn type_constraint(table: &TableAlias, tag: i32, to_check: Option<Vec<SQLTypeAffinity>>) -> Constraint {
+    let type_column = QualifiedAlias::new(table.clone(),
+                                          DatomsColumn::ValueTypeTag).to_column();
+    let check_type_tag = Constraint::equal(type_column, ColumnOrExpression::Integer(tag));
+    if let Some(affinities) = to_check {
+        let check_affinities = Constraint::Or {
+            constraints: affinities.into_iter().map(|affinity| {
+                Constraint::TypeCheck {
+                    value: QualifiedAlias::new(table.clone(),
+                                               DatomsColumn::Value).to_column(),
+                    affinity,
+                }
+            }).collect()
+        };
+        Constraint::And {
+            constraints: vec![
+                check_type_tag,
+                check_affinities
+            ]
+        }
+    } else {
+        check_type_tag
+    }
+}
+
+// Returns a map of tags to a vector of all the possible affinities that those tags can represent
+// given the types in `value_types`.
+fn possible_affinities(value_types: ValueTypeSet) -> HashMap<i32, Vec<SQLTypeAffinity>> {
+    let mut result = HashMap::new();
+    for ty in value_types {
+        let (tag, affinity_to_check) = ty.sql_representation();
+        let mut affinities = result.entry(tag).or_insert_with(Vec::new);
+        if let Some(affinity) = affinity_to_check {
+            affinities.push(affinity);
+        }
+    }
+    result
+}
+
 impl ToConstraint for ColumnConstraint {
     fn to_constraint(self) -> Constraint {
         use self::ColumnConstraint::*;
@@ -159,52 +206,23 @@ impl ToConstraint for ColumnConstraint {
                     right: right.into(),
                 }
             },
-            HasTypes { value: table, value_types, strict: strict_requested } => {
+            HasTypes { value: table, value_types, check_value } => {
-                // If strict mode checking is on, and need to check exactly 1
+                let constraints = if check_value {
-                // (not both or neither) of ValueType::Double and ValueType::Long
+                    possible_affinities(value_types)
-                // we emit a Constraint::TypeCheck.
+                        .into_iter()
-                let num_numeric_checks = (value_types.contains(ValueType::Double) as i32) +
+                        .map(|(tag, affinities)| {
-                                         (value_types.contains(ValueType::Long) as i32);
+                            let to_check = if affinities.len() == 0 || affinities.len() == affinity_count(tag) {
-                let strict = strict_requested && num_numeric_checks == 1;
+                                None
-
+                            } else {
-                let types = if !strict && num_numeric_checks == 2 {
+                                Some(affinities)
-                    // If we aren't operating in strict mode (either because it
+                            };
-                    // wasn't requested, or because it doesn't make a difference),
+                            type_constraint(&table, tag, to_check)
-                    // and both ValueType::Double and ValueType::Long are being
+                        }).collect()
-                    // checked, we remove the test for one of them, as they're
-                    // represented using the same value type tag (we choose to
-                    // remove the check for ValueType::Double, but it's an
-                    // arbitrary choice)
-                    value_types.difference(&ValueTypeSet::of_one(ValueType::Double))
                 } else {
-                    value_types
+                    value_types.into_iter()
+                               .map(|vt| type_constraint(&table, vt.value_type_tag(), None))
+                               .collect()
                 };
-                let constraints = types.into_iter().map(|ty| {
-                    let type_column = QualifiedAlias::new(table.clone(), DatomsColumn::ValueTypeTag).to_column();
-                    let loose = Constraint::equal(type_column, ColumnOrExpression::Integer(ty.value_type_tag()));
-                    if !strict || (ty != ValueType::Long && ty != ValueType::Double) {
-                        loose
-                    } else {
-                        let val_column = QualifiedAlias::new(table.clone(), DatomsColumn::Value).to_column();
-                        // We're handling strict equality for a numeric type, so we need to emit
-                        // a `typeof(col) = '(real|integer)'` too. This should happen a maximum of
-                        // once per `HasTypes`
-                        Constraint::And {
-                            constraints: vec![
-                                loose,
-                                Constraint::TypeCheck {
-                                    value: val_column,
-                                    affinity: match ty {
-                                        ValueType::Long => SQLTypeAffinity::Integer,
-                                        ValueType::Double => SQLTypeAffinity::Real,
-                                        _ => unreachable!()
-                                    }
-                                }
-                            ]
-                        }
-                    }
-                }).collect::<Vec<_>>();
-
                 Constraint::Or { constraints }
             },
 

query-translator/tests/translate.rs

@@ -296,7 +296,7 @@ fn test_type_required_long() {
     assert_eq!(sql, "SELECT DISTINCT `datoms00`.e AS `?x` \
                      FROM `datoms` AS `datoms00` \
                      WHERE ((`datoms00`.value_type_tag = 5 AND \
-                             typeof(`datoms00`.v) = 'integer'))");
+                             (typeof(`datoms00`.v) = 'integer')))");
 
     assert_eq!(args, vec![]);
 }
@@ -311,7 +311,7 @@ fn test_type_required_double() {
     assert_eq!(sql, "SELECT DISTINCT `datoms00`.e AS `?x` \
                      FROM `datoms` AS `datoms00` \
                      WHERE ((`datoms00`.value_type_tag = 5 AND \
-                             typeof(`datoms00`.v) = 'real'))");
+                             (typeof(`datoms00`.v) = 'real')))");
 
     assert_eq!(args, vec![]);
 }