Cleans up the Variable structure to only represent actual variables

There should be now a sane blank node support in the SPARQL algebra.
They are now allowed only in places where they are allowed by the SPARQL syntax

js/src/store.rs

@@ -118,10 +118,8 @@ impl JsMemoryStore {
         let output = match results {
             QueryResult::Bindings(bindings) => {
                 let (variables, iter) = bindings.destruct();
-                let variables: Vec<JsValue> = variables
-                    .into_iter()
-                    .map(|v| v.name().unwrap().into())
-                    .collect();
+                let variables: Vec<JsValue> =
+                    variables.into_iter().map(|v| v.as_str().into()).collect();
                 let results = Array::new();
                 for values in iter {
                     let values = values.map_err(to_err)?;

lib/src/sparql/algebra.rs

@@ -244,7 +244,9 @@ impl<'a> fmt::Display for SparqlTripleOrPathPattern<'a> {
 
 #[derive(Eq, PartialEq, Ord, PartialOrd, Debug, Clone, Hash)]
 pub enum Expression {
-    Constant(TermOrVariable),
+    NamedNode(NamedNode),
+    Literal(Literal),
+    Variable(Variable),
     Or(Box<Expression>, Box<Expression>),
     And(Box<Expression>, Box<Expression>),
     Equal(Box<Expression>, Box<Expression>),
@@ -270,7 +272,9 @@ pub enum Expression {
 impl fmt::Display for Expression {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
-            Expression::Constant(t) => write!(f, "{}", t),
+            Expression::NamedNode(node) => node.fmt(f),
+            Expression::Literal(l) => l.fmt(f),
+            Expression::Variable(var) => var.fmt(f),
             Expression::Or(a, b) => write!(f, "({} || {})", a, b),
             Expression::And(a, b) => write!(f, "({} && {})", a, b),
             Expression::Equal(a, b) => write!(f, "({} = {})", a, b),
@@ -324,19 +328,19 @@ impl fmt::Display for Expression {
 
 impl From<NamedNode> for Expression {
     fn from(p: NamedNode) -> Self {
-        Expression::Constant(p.into())
+        Expression::NamedNode(p)
     }
 }
 
 impl From<Literal> for Expression {
     fn from(p: Literal) -> Self {
-        Expression::Constant(p.into())
+        Expression::Literal(p)
     }
 }
 
 impl From<Variable> for Expression {
     fn from(v: Variable) -> Self {
-        Expression::Constant(v.into())
+        Expression::Variable(v)
     }
 }
 
@@ -345,7 +349,9 @@ struct SparqlExpression<'a>(&'a Expression);
 impl<'a> fmt::Display for SparqlExpression<'a> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self.0 {
-            Expression::Constant(t) => write!(f, "{}", t),
+            Expression::NamedNode(node) => node.fmt(f),
+            Expression::Literal(l) => l.fmt(f),
+            Expression::Variable(var) => var.fmt(f),
             Expression::Or(a, b) => write!(
                 f,
                 "({} || {})",
@@ -677,21 +683,21 @@ impl GraphPattern {
                     match pattern {
                         TripleOrPathPattern::Triple(tp) => {
                             if let TermOrVariable::Variable(ref s) = tp.subject {
-                                adds_if_has_name(vars, s);
+                                vars.insert(s);
                             }
                             if let NamedNodeOrVariable::Variable(ref p) = tp.predicate {
-                                adds_if_has_name(vars, p);
+                                vars.insert(p);
                             }
                             if let TermOrVariable::Variable(ref o) = tp.object {
-                                adds_if_has_name(vars, o);
+                                vars.insert(o);
                             }
                         }
                         TripleOrPathPattern::Path(ppp) => {
                             if let TermOrVariable::Variable(ref s) = ppp.subject {
-                                adds_if_has_name(vars, s);
+                                vars.insert(s);
                             }
                             if let TermOrVariable::Variable(ref o) = ppp.object {
-                                adds_if_has_name(vars, o);
+                                vars.insert(o);
                             }
                         }
                     }
@@ -712,17 +718,21 @@ impl GraphPattern {
             }
             GraphPattern::Graph(g, p) => {
                 if let NamedNodeOrVariable::Variable(ref g) = g {
-                    adds_if_has_name(vars, g);
+                    vars.insert(g);
                 }
                 p.add_visible_variables(vars);
             }
             GraphPattern::Extend(p, v, _) => {
+                vars.insert(v);
                 p.add_visible_variables(vars);
-                adds_if_has_name(vars, v);
             }
             GraphPattern::Minus(a, _) => a.add_visible_variables(vars),
             GraphPattern::Service(_, p, _) => p.add_visible_variables(vars),
-            GraphPattern::AggregateJoin(_, a) => vars.extend(a.iter().map(|(_, v)| v)),
+            GraphPattern::AggregateJoin(_, a) => {
+                for v in a.values() {
+                    vars.insert(v);
+                }
+            }
             GraphPattern::Data(b) => vars.extend(b.variables_iter()),
             GraphPattern::OrderBy(l, _) => l.add_visible_variables(vars),
             GraphPattern::Project(_, pv) => vars.extend(pv.iter()),
@@ -733,12 +743,6 @@ impl GraphPattern {
     }
 }
 
-fn adds_if_has_name<'a>(vars: &mut BTreeSet<&'a Variable>, var: &'a Variable) {
-    if var.has_name() {
-        vars.insert(var);
-    }
-}
-
 struct SparqlGraphPattern<'a>(&'a GraphPattern);
 
 impl<'a> fmt::Display for SparqlGraphPattern<'a> {

lib/src/sparql/json_results.rs

@@ -23,7 +23,7 @@ pub fn write_json_results<W: Write>(results: QueryResult<'_>, mut sink: W) -> Re
                 } else {
                     sink.write_all(b",")?;
                 }
-                write_escaped_json_string(variable.name()?, &mut sink)?;
+                write_escaped_json_string(variable.as_str(), &mut sink)?;
             }
             sink.write_all(b"]},\"results\":{\"bindings\":[")?;
             let mut start_bindings = true;
@@ -44,7 +44,7 @@ pub fn write_json_results<W: Write>(results: QueryResult<'_>, mut sink: W) -> Re
                         } else {
                             sink.write_all(b",")?;
                         }
-                        write_escaped_json_string(variables[i].name()?, &mut sink)?;
+                        write_escaped_json_string(variables[i].as_str(), &mut sink)?;
                         match term {
                             Term::NamedNode(uri) => {
                                 sink.write_all(b":{\"type\":\"uri\",\"value\":")?;

lib/src/sparql/model.rs

@@ -153,55 +153,36 @@ impl<'a> BindingsIterator<'a> {
 
 /// A SPARQL query variable
 #[derive(Eq, PartialEq, Ord, PartialOrd, Debug, Clone, Hash)]
-pub enum Variable {
-    Variable { name: String },
-    BlankNode { id: u128 },
-    Internal { id: u128 },
+pub struct Variable {
+    name: String,
 }
 
 impl Variable {
     pub fn new(name: impl Into<String>) -> Self {
-        Variable::Variable { name: name.into() }
+        Variable { name: name.into() }
     }
 
-    pub fn has_name(&self) -> bool {
-        match self {
-            Variable::Variable { .. } => true,
-            _ => false,
-        }
+    pub fn as_str(&self) -> &str {
+        &self.name
     }
 
+    #[deprecated]
     pub fn name(&self) -> Result<&str> {
-        match self {
-            Variable::Variable { name } => Ok(name),
-            _ => Err(Error::msg(format!("The variable {} has no name", self))),
-        }
-    }
+        Ok(self.as_str())
     }
 
-impl fmt::Display for Variable {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            Variable::Variable { name } => write!(f, "?{}", name),
-            Variable::BlankNode { id } => write!(f, "_:{:x}", id),
-            Variable::Internal { id } => write!(f, "?{:x}", id),
-        }
-    }
+    pub fn into_string(self) -> String {
+        self.name
     }
 
-impl Default for Variable {
-    fn default() -> Self {
-        Variable::Internal {
-            id: random::<u128>(),
-        }
+    pub(crate) fn new_random() -> Self {
+        Self::new(format!("{:x}", random::<u128>()))
     }
 }
 
-impl From<BlankNode> for Variable {
-    fn from(blank_node: BlankNode) -> Self {
-        Variable::BlankNode {
-            id: blank_node.id(),
-        }
+impl fmt::Display for Variable {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "?{}", self.name)
     }
 }
 
@@ -214,8 +195,8 @@ pub enum NamedNodeOrVariable {
 impl fmt::Display for NamedNodeOrVariable {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
-            NamedNodeOrVariable::NamedNode(node) => write!(f, "{}", node),
-            NamedNodeOrVariable::Variable(var) => write!(f, "{}", var),
+            NamedNodeOrVariable::NamedNode(node) => node.fmt(f),
+            NamedNodeOrVariable::Variable(var) => var.fmt(f),
         }
     }
 }
@@ -241,8 +222,8 @@ pub enum TermOrVariable {
 impl fmt::Display for TermOrVariable {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
-            TermOrVariable::Term(term) => write!(f, "{}", term),
-            TermOrVariable::Variable(var) => write!(f, "{}", var),
+            TermOrVariable::Term(term) => term.fmt(f),
+            TermOrVariable::Variable(var) => var.fmt(f),
         }
     }
 }
@@ -255,7 +236,7 @@ impl From<NamedNode> for TermOrVariable {
 
 impl From<BlankNode> for TermOrVariable {
     fn from(node: BlankNode) -> Self {
-        TermOrVariable::Variable(node.into())
+        TermOrVariable::Term(node.into())
     }
 }
 
@@ -273,11 +254,7 @@ impl From<Variable> for TermOrVariable {
 
 impl From<Term> for TermOrVariable {
     fn from(term: Term) -> Self {
-        match term {
-            Term::NamedNode(node) => TermOrVariable::Term(node.into()),
-            Term::BlankNode(node) => TermOrVariable::Variable(node.into()),
-            Term::Literal(literal) => TermOrVariable::Term(literal.into()),
-        }
+        TermOrVariable::Term(term)
     }
 }
 

lib/src/sparql/parser.rs

@@ -80,7 +80,7 @@ fn add_to_triple_or_path_patterns(
                 add_to_triple_or_path_patterns(object, *p, subject, patterns)
             }
             PropertyPath::SequencePath(a, b) => {
-                let middle = Variable::default();
+                let middle = BlankNode::default();
                 add_to_triple_or_path_patterns(subject, *a, middle.clone().into(), patterns);
                 add_to_triple_or_path_patterns(middle.into(), *b, object, patterns);
             }
@@ -213,7 +213,7 @@ fn build_select(
     //GROUP BY
     let aggregations = state.aggregations.pop().unwrap_or_else(BTreeMap::default);
     if group.is_none() && !aggregations.is_empty() {
-        let const_variable = Variable::default();
+        let const_variable = Variable::new_random();
         group = Some((
             vec![const_variable.clone()],
             vec![(Literal::from(1).into(), const_variable)],
@@ -310,7 +310,7 @@ impl ParserState {
             .last_mut()
             .ok_or_else(|| "Unexpected aggregate")?;
         Ok(aggregations.get(&agg).cloned().unwrap_or_else(|| {
-            let new_var = Variable::default();
+            let new_var = Variable::new_random();
             aggregations.insert(agg, new_var.clone());
             new_var
         }))
@@ -644,7 +644,7 @@ parser! {
                     algebra: build_select(Selection {
                         option: SelectionOption::Default,
                         variables: Some(p.into_iter().map(|var_or_iri| match var_or_iri {
-                            NamedNodeOrVariable::NamedNode(n) => SelectionMember::Expression(n.into(), Variable::default()),
+                            NamedNodeOrVariable::NamedNode(n) => SelectionMember::Expression(n.into(), Variable::new_random()),
                             NamedNodeOrVariable::Variable(v) => SelectionMember::Variable(v)
                         }).collect())
                     }, w.unwrap_or_else(GraphPattern::default), g, h, o, l, v, state),
@@ -690,10 +690,10 @@ parser! {
         rule GroupClause() -> (Vec<Variable>, Vec<(Expression,Variable)>) = i("GROUP") _ i("BY") _ c:GroupCondition_item()+ {
             let mut projections: Vec<(Expression,Variable)> = Vec::default();
             let clauses = c.into_iter().map(|(e, vo)| {
-                if let Expression::Constant(TermOrVariable::Variable(v)) = e {
+                if let Expression::Variable(v) = e {
                     v
                 } else {
-                    let v = vo.unwrap_or_else(Variable::default);
+                    let v = vo.unwrap_or_else(Variable::new_random);
                     projections.push((e, v.clone()));
                     v
                 }
@@ -1310,10 +1310,10 @@ parser! {
         rule PrimaryExpression() -> Expression =
             BrackettedExpression() /
             iriOrFunction() /
-            v:Var() { Expression::Constant(v.into()) } /
-            l:RDFLiteral() { Expression::Constant(l.into()) } /
-            l:NumericLiteral() { Expression::Constant(l.into()) } /
-            l:BooleanLiteral() { Expression::Constant(l.into()) } /
+            v:Var() { v.into() } /
+            l:RDFLiteral() { l.into() } /
+            l:NumericLiteral() { l.into() } /
+            l:BooleanLiteral() { l.into() } /
             BuiltInCall()
 
         //[120]
@@ -1423,7 +1423,7 @@ parser! {
         rule iriOrFunction() -> Expression = i: iri() _ a: ArgList()? {
             match a {
                 Some(a) => Expression::FunctionCall(Function::Custom(i), a),
-                None => Expression::Constant(i.into())
+                None => i.into()
             }
         }
 

lib/src/sparql/plan_builder.rs

@@ -1,6 +1,6 @@
+use crate::model::{BlankNode, Term};
 use crate::sparql::algebra::*;
 use crate::sparql::model::*;
-use crate::sparql::plan::PlanPropertyPath;
 use crate::sparql::plan::*;
 use crate::store::numeric_encoder::{Encoder, ENCODED_DEFAULT_GRAPH};
 use crate::Error;
@@ -276,10 +276,11 @@ impl<E: Encoder> PlanBuilder<E> {
         graph_name: PatternValue,
     ) -> Result<PlanExpression> {
         Ok(match expression {
-            Expression::Constant(t) => match t {
-                TermOrVariable::Term(t) => PlanExpression::Constant(self.encoder.encode_term(t)?),
-                TermOrVariable::Variable(v) => PlanExpression::Variable(variable_key(variables, v)),
-            },
+            Expression::NamedNode(node) => {
+                PlanExpression::Constant(self.encoder.encode_named_node(node)?)
+            }
+            Expression::Literal(l) => PlanExpression::Constant(self.encoder.encode_literal(l)?),
+            Expression::Variable(v) => PlanExpression::Variable(variable_key(variables, v)),
             Expression::Or(a, b) => PlanExpression::Or(
                 Box::new(self.build_for_expression(a, variables, graph_name)?),
                 Box::new(self.build_for_expression(b, variables, graph_name)?),
@@ -704,10 +705,14 @@ impl<E: Encoder> PlanBuilder<E> {
         variables: &mut Vec<Variable>,
     ) -> Result<PatternValue> {
         Ok(match term_or_variable {
-            TermOrVariable::Term(term) => PatternValue::Constant(self.encoder.encode_term(term)?),
             TermOrVariable::Variable(variable) => {
                 PatternValue::Variable(variable_key(variables, variable))
             }
+            TermOrVariable::Term(Term::BlankNode(bnode)) => {
+                PatternValue::Variable(variable_key(variables, &Variable::new(bnode.as_str())))
+                //TODO: very bad hack to convert bnode to variable
+            }
+            TermOrVariable::Term(term) => PatternValue::Constant(self.encoder.encode_term(term)?),
         })
     }
 
@@ -818,11 +823,8 @@ impl<E: Encoder> PlanBuilder<E> {
                         variables,
                         &mut bnodes,
                     )?,
-                    predicate: self.template_value_from_named_node_or_variable(
-                        &triple.predicate,
-                        variables,
-                        &mut bnodes,
-                    )?,
+                    predicate: self
+                        .template_value_from_named_node_or_variable(&triple.predicate, variables)?,
                     object: self.template_value_from_term_or_variable(
                         &triple.object,
                         variables,
@@ -837,18 +839,17 @@ impl<E: Encoder> PlanBuilder<E> {
         &mut self,
         term_or_variable: &TermOrVariable,
         variables: &mut Vec<Variable>,
-        bnodes: &mut Vec<Variable>,
+        bnodes: &mut Vec<BlankNode>,
     ) -> Result<TripleTemplateValue> {
         Ok(match term_or_variable {
-            TermOrVariable::Term(term) => {
-                TripleTemplateValue::Constant(self.encoder.encode_term(term)?)
-            }
             TermOrVariable::Variable(variable) => {
-                if variable.has_name() {
                 TripleTemplateValue::Variable(variable_key(variables, variable))
-                } else {
-                    TripleTemplateValue::BlankNode(variable_key(bnodes, variable))
             }
+            TermOrVariable::Term(Term::BlankNode(bnode)) => {
+                TripleTemplateValue::BlankNode(bnode_key(bnodes, bnode))
+            }
+            TermOrVariable::Term(term) => {
+                TripleTemplateValue::Constant(self.encoder.encode_term(term)?)
             }
         })
     }
@@ -857,18 +858,13 @@ impl<E: Encoder> PlanBuilder<E> {
         &mut self,
         named_node_or_variable: &NamedNodeOrVariable,
         variables: &mut Vec<Variable>,
-        bnodes: &mut Vec<Variable>,
     ) -> Result<TripleTemplateValue> {
         Ok(match named_node_or_variable {
-            NamedNodeOrVariable::NamedNode(term) => {
-                TripleTemplateValue::Constant(self.encoder.encode_named_node(term)?)
-            }
             NamedNodeOrVariable::Variable(variable) => {
-                if variable.has_name() {
                 TripleTemplateValue::Variable(variable_key(variables, variable))
-                } else {
-                    TripleTemplateValue::BlankNode(variable_key(bnodes, variable))
             }
+            NamedNodeOrVariable::NamedNode(term) => {
+                TripleTemplateValue::Constant(self.encoder.encode_named_node(term)?)
             }
         })
     }
@@ -902,7 +898,7 @@ impl<E: Encoder> PlanBuilder<E> {
         }) {
             to_id
         } else {
-            to.push(Variable::default());
+            to.push(Variable::new_random());
             to.len() - 1
         }
     }
@@ -981,6 +977,16 @@ fn variable_key(variables: &mut Vec<Variable>, variable: &Variable) -> usize {
     }
 }
 
+fn bnode_key(blank_nodes: &mut Vec<BlankNode>, blank_node: &BlankNode) -> usize {
+    match slice_key(blank_nodes, blank_node) {
+        Some(key) => key,
+        None => {
+            blank_nodes.push(blank_node.clone());
+            blank_nodes.len() - 1
+        }
+    }
+}
+
 fn slice_key<T: Eq>(slice: &[T], element: &T) -> Option<usize> {
     for (i, item) in slice.iter().enumerate() {
         if item == element {
@@ -992,14 +998,16 @@ fn slice_key<T: Eq>(slice: &[T], element: &T) -> Option<usize> {
 
 fn sort_bgp(p: &[TripleOrPathPattern]) -> Vec<&TripleOrPathPattern> {
     let mut assigned_variables = BTreeSet::default();
+    let mut assigned_blank_nodes = BTreeSet::default();
     let mut new_p: Vec<_> = p.iter().collect();
 
     for i in 0..new_p.len() {
         (&mut new_p[i..]).sort_by(|p1, p2| {
-            count_pattern_binds(p2, &assigned_variables)
-                .cmp(&count_pattern_binds(p1, &assigned_variables))
+            count_pattern_binds(p2, &assigned_variables, &assigned_blank_nodes).cmp(
+                &count_pattern_binds(p1, &assigned_variables, &assigned_blank_nodes),
+            )
         });
-        add_pattern_variables(new_p[i], &mut assigned_variables);
+        add_pattern_variables(new_p[i], &mut assigned_variables, &mut assigned_blank_nodes);
     }
 
     new_p
@@ -1008,12 +1016,17 @@ fn sort_bgp(p: &[TripleOrPathPattern]) -> Vec<&TripleOrPathPattern> {
 fn count_pattern_binds(
     pattern: &TripleOrPathPattern,
     assigned_variables: &BTreeSet<&Variable>,
+    assigned_blank_nodes: &BTreeSet<&BlankNode>,
 ) -> u8 {
     let mut count = 12;
     if let TermOrVariable::Variable(v) = pattern.subject() {
         if !assigned_variables.contains(v) {
             count -= 4;
         }
+    } else if let TermOrVariable::Term(Term::BlankNode(bnode)) = pattern.subject() {
+        if !assigned_blank_nodes.contains(bnode) {
+            count -= 4;
+        }
     } else {
         count -= 1;
     }
@@ -1032,6 +1045,10 @@ fn count_pattern_binds(
         if !assigned_variables.contains(v) {
             count -= 4;
         }
+    } else if let TermOrVariable::Term(Term::BlankNode(bnode)) = pattern.object() {
+        if !assigned_blank_nodes.contains(bnode) {
+            count -= 4;
+        }
     } else {
         count -= 1;
     }
@@ -1041,9 +1058,12 @@ fn count_pattern_binds(
 fn add_pattern_variables<'a>(
     pattern: &'a TripleOrPathPattern,
     variables: &mut BTreeSet<&'a Variable>,
+    blank_nodes: &mut BTreeSet<&'a BlankNode>,
 ) {
     if let TermOrVariable::Variable(v) = pattern.subject() {
         variables.insert(v);
+    } else if let TermOrVariable::Term(Term::BlankNode(bnode)) = pattern.subject() {
+        blank_nodes.insert(bnode);
     }
     if let TripleOrPathPattern::Triple(t) = pattern {
         if let NamedNodeOrVariable::Variable(v) = &t.predicate {
@@ -1052,5 +1072,7 @@ fn add_pattern_variables<'a>(
     }
     if let TermOrVariable::Variable(v) = pattern.object() {
         variables.insert(v);
+    } else if let TermOrVariable::Term(Term::BlankNode(bnode)) = pattern.object() {
+        blank_nodes.insert(bnode);
     }
 }

lib/src/sparql/xml_results.rs

@@ -44,7 +44,7 @@ pub fn write_xml_results<W: Write>(results: QueryResult<'_>, sink: W) -> Result<
             writer.write_event(Event::Start(BytesStart::borrowed_name(b"head")))?;
             for variable in &variables {
                 let mut variable_tag = BytesStart::borrowed_name(b"variable");
-                variable_tag.push_attribute(("name", variable.name()?));
+                variable_tag.push_attribute(("name", variable.as_str()));
                 writer.write_event(Event::Empty(variable_tag))?;
             }
             writer.write_event(Event::End(BytesEnd::borrowed(b"head")))?;
@@ -55,7 +55,7 @@ pub fn write_xml_results<W: Write>(results: QueryResult<'_>, sink: W) -> Result<
                 for (i, value) in result.into_iter().enumerate() {
                     if let Some(term) = value {
                         let mut binding_tag = BytesStart::borrowed_name(b"binding");
-                        binding_tag.push_attribute(("name", variables[i].name()?));
+                        binding_tag.push_attribute(("name", variables[i].as_str()));
                         writer.write_event(Event::Start(binding_tag))?;
                         match term {
                             Term::NamedNode(uri) => {

lib/tests/sparql_test_cases.rs

@@ -361,7 +361,7 @@ fn to_graph(result: QueryResult<'_>, with_order: bool) -> Result<SimpleGraph> {
                 graph.insert(Triple::new(
                     result_set,
                     rs::RESULT_VARIABLE.clone(),
-                    Literal::new_simple_literal(variable.name()?),
+                    Literal::new_simple_literal(variable.as_str()),
                 ));
             }
             for (i, binding_values) in iter.enumerate() {
@@ -376,7 +376,7 @@ fn to_graph(result: QueryResult<'_>, with_order: bool) -> Result<SimpleGraph> {
                         graph.insert(Triple::new(
                             binding,
                             rs::VARIABLE.clone(),
-                            Literal::new_simple_literal(variables[i].name()?),
+                            Literal::new_simple_literal(variables[i].as_str()),
                         ));
                     }
                 }