made min/max aggregate functions support all literals (#694)

not just numerics as before.

fixes #628

9 files changed, 84 insertions, 50 deletions

diff --git a/rdflib/plugins/sparql/aggregates.py b/rdflib/plugins/sparql/aggregates.py
index 8716adf1..7ffe1de4 100644
--- a/rdflib/plugins/sparql/aggregates.py
+++ b/rdflib/plugins/sparql/aggregates.py
@@ -1,10 +1,9 @@
 from rdflib import Literal, XSD
 
-from rdflib.plugins.sparql.evalutils import _eval, NotBoundError
+from rdflib.plugins.sparql.evalutils import _eval, NotBoundError, _val
 from rdflib.plugins.sparql.operators import numeric
 from rdflib.plugins.sparql.datatypes import type_promotion
 
-from rdflib.plugins.sparql.compat import num_max, num_min
 from rdflib.plugins.sparql.sparql import SPARQLTypeError
 
 from decimal import Decimal
@@ -157,10 +156,10 @@ class Extremum(Accumulator):
     def update(self, row, aggregator):
         try:
             if self.value is None:
-                self.value = numeric(_eval(self.expr, row))
+                self.value = _eval(self.expr, row)
             else:
                 # self.compare is implemented by Minimum/Maximum
-                self.value = self.compare(self.value, numeric(_eval(self.expr, row)))
+                self.value = self.compare(self.value, _eval(self.expr, row))
         # skip UNDEF or BNode => SPARQLTypeError
         except NotBoundError:
             pass
@@ -171,13 +170,13 @@ class Extremum(Accumulator):
 class Minimum(Extremum):
 
     def compare(self, val1, val2):
-        return num_min(val1, val2)
+        return min(val1, val2, key=_val)
 
 
 class Maximum(Extremum):
 
     def compare(self, val1, val2):
-        return num_max(val1, val2)
+        return max(val1, val2, key=_val)
 
 
 class Sample(Accumulator):
@@ -199,7 +198,7 @@ class Sample(Accumulator):
 
     def get_value(self):
         # set None if no value was set
-        return None 
+        return None
 
 class GroupConcat(Accumulator):
 
@@ -247,8 +246,9 @@ class Aggregator(object):
 
     def update(self, row):
         """update all own accumulators"""
-        # SAMPLE accumulators may delete themselves 
+        # SAMPLE accumulators may delete themselves
         # => iterate over list not generator
+
         for acc in self.accumulators.values():
             if acc.use_row(row):
                 acc.update(row, self)
@@ -258,4 +258,3 @@ class Aggregator(object):
         for acc in self.accumulators.itervalues():
             acc.set_value(self.bindings)
         return self.bindings
-
diff --git a/rdflib/plugins/sparql/compat.py b/rdflib/plugins/sparql/compat.py
index 01147b70..bb8e250a 100644
--- a/rdflib/plugins/sparql/compat.py
+++ b/rdflib/plugins/sparql/compat.py
@@ -23,29 +23,3 @@ try:
     from collections import OrderedDict  # was added in 2.7
 except ImportError:
     from ordereddict import OrderedDict  # extra module
-
-import sys
-
-if sys.version_info[:2] < (2, 7):
-
-    from decimal import Decimal
-    # Pre-2.7 decimal and float did not compare correctly
-
-    def _numericKey(n):
-        if isinstance(n, Decimal):
-            return float(n)
-        else:
-            return n
-
-    def num_max(*args, **kwargs):
-        kwargs["key"] = _numericKey
-        return max(*args, **kwargs)
-
-    def num_min(*args, **kwargs):
-        kwargs["key"] = _numericKey
-        return min(*args, **kwargs)
-
-else:
-
-    num_max = max
-    num_min = min
diff --git a/rdflib/plugins/sparql/evaluate.py b/rdflib/plugins/sparql/evaluate.py
index 8f016a16..6b59f532 100644
--- a/rdflib/plugins/sparql/evaluate.py
+++ b/rdflib/plugins/sparql/evaluate.py
@@ -23,7 +23,7 @@ from rdflib.plugins.sparql.parserutils import value
 from rdflib.plugins.sparql.sparql import (
     QueryContext, AlreadyBound, FrozenBindings, SPARQLError)
 from rdflib.plugins.sparql.evalutils import (
-    _filter, _eval, _join, _diff, _minus, _fillTemplate, _ebv)
+    _filter, _eval, _join, _diff, _minus, _fillTemplate, _ebv, _val)
 
 from rdflib.plugins.sparql.aggregates import Aggregator
 from rdflib.plugins.sparql.algebra import Join, ToMultiSet, Values
@@ -316,19 +316,8 @@ def evalOrderBy(ctx, part):
 
     for e in reversed(part.expr):
 
-        def val(x):
-            v = value(x, e.expr, variables=True)
-            if isinstance(v, Variable):
-                return (0, v)
-            elif isinstance(v, BNode):
-                return (1, v)
-            elif isinstance(v, URIRef):
-                return (2, v)
-            elif isinstance(v, Literal):
-                return (3, v)
-
         reverse = bool(e.order and e.order == 'DESC')
-        res = sorted(res, key=val, reverse=reverse)
+        res = sorted(res, key=lambda x: _val(value(x, e.expr, variables=True)), reverse=reverse)
 
     return res
 
@@ -366,7 +355,7 @@ def evalReduced(ctx, part):
     MAX = 1
     # TODO: add configuration or determine "best" size for most use cases
     # 0: No reduction
-    # 1: compare only with the last row, almost no reduction with 
+    # 1: compare only with the last row, almost no reduction with
     #    unordered incoming rows
     # N: The greater the buffer size the greater the reduction but more
     #    memory and time are needed
diff --git a/rdflib/plugins/sparql/evalutils.py b/rdflib/plugins/sparql/evalutils.py
index ddfe2311..dd0924d0 100644
--- a/rdflib/plugins/sparql/evalutils.py
+++ b/rdflib/plugins/sparql/evalutils.py
@@ -3,7 +3,7 @@ import collections
 from rdflib.term import Variable, Literal, BNode, URIRef
 
 from rdflib.plugins.sparql.operators import EBV
-from rdflib.plugins.sparql.parserutils import Expr, CompValue
+from rdflib.plugins.sparql.parserutils import Expr, CompValue, value
 from rdflib.plugins.sparql.sparql import SPARQLError, NotBoundError
 
 
@@ -112,3 +112,14 @@ def _fillTemplate(template, solution):
                 _o is not None:
 
             yield (_s, _p, _o)
+
+def _val(v):
+    """ utilitity for ordering things"""
+    if isinstance(v, Variable):
+        return (0, v)
+    elif isinstance(v, BNode):
+        return (1, v)
+    elif isinstance(v, URIRef):
+        return (2, v)
+    elif isinstance(v, Literal):
+        return (3, v)
diff --git a/test/DAWG/rdflib/manifest.ttl b/test/DAWG/rdflib/manifest.ttl
index 87ecdcad..85ba2db5 100644
--- a/test/DAWG/rdflib/manifest.ttl
+++ b/test/DAWG/rdflib/manifest.ttl
@@ -16,8 +16,10 @@
     :whitespacedot
     :minusfilter
     :notexistsfilter
+
     :bindscope
     :bindscope2
+
     :filtersubquery1
     :filtersubquery2
     :filtersubquery3
@@ -27,6 +29,9 @@
     :subquery2
     :subquery3
 
+    :subqueryagg1
+    :subqueryagg2
+
 	) .
 
 
@@ -213,3 +218,31 @@ From https://github.com/RDFLib/rdflib/issues/615, contributed by https://github.
            qt:data <subquery.ttl> ] ;
     mf:result  <subquery.tsv>
     .
+
+
+:subqueryagg1 rdf:type mf:QueryEvaluationTest ;
+    mf:name "plain query aggregation function";
+    rdfs:comment """
+    From https://github.com/RDFLib/rdflib/issues/628, contributed by https://github.com/wrobell
+""";
+    dawgt:approval dawgt:Approved ;
+    dawgt:approvedBy <http://gromgull.net/me> ;
+    mf:action
+         [ qt:query <subqueryagg1.rq> ;
+           qt:data <subqueryagg.ttl> ] ;
+    mf:result  <subqueryagg.tsv>
+    .
+
+
+:subqueryagg2 rdf:type mf:QueryEvaluationTest ;
+    mf:name "subquery aggregation function";
+    rdfs:comment """
+    From https://github.com/RDFLib/rdflib/issues/628, contributed by https://github.com/wrobell
+""";
+    dawgt:approval dawgt:Approved ;
+    dawgt:approvedBy <http://gromgull.net/me> ;
+    mf:action
+         [ qt:query <subqueryagg.rq> ;
+           qt:data <subqueryagg.ttl> ] ;
+    mf:result  <subqueryagg.tsv>
+    .
diff --git a/test/DAWG/rdflib/subqueryagg.rq b/test/DAWG/rdflib/subqueryagg.rq
new file mode 100644
index 00000000..3ee1a3e3
--- /dev/null
+++ b/test/DAWG/rdflib/subqueryagg.rq
@@ -0,0 +1,12 @@
+PREFIX : <http://people.example/>
+
+SELECT ?y ?minName
+WHERE {
+  :alice :knows ?y .
+  {
+    SELECT ?y (MIN(?name) AS ?minName)
+    WHERE {
+      ?y :name ?name .
+    } GROUP BY ?y
+  }
+}
diff --git a/test/DAWG/rdflib/subqueryagg.tsv b/test/DAWG/rdflib/subqueryagg.tsv
new file mode 100644
index 00000000..c6460797
--- /dev/null
+++ b/test/DAWG/rdflib/subqueryagg.tsv
@@ -0,0 +1,3 @@
+?y	?minName
+<http://people.example/bob>	"B. Bar"
+<http://people.example/carol>	"C. Baz"
diff --git a/test/DAWG/rdflib/subqueryagg.ttl b/test/DAWG/rdflib/subqueryagg.ttl
new file mode 100644
index 00000000..20271065
--- /dev/null
+++ b/test/DAWG/rdflib/subqueryagg.ttl
@@ -0,0 +1,6 @@
+@prefix : <http://people.example/> .
+
+:alice :name "Alice", "Alice Foo", "A. Foo" .
+:alice :knows :bob, :carol .
+:bob :name "Bob", "Bob Bar", "B. Bar" .
+:carol :name "Carol", "Carol Baz", "C. Baz" .
diff --git a/test/DAWG/rdflib/subqueryagg1.rq b/test/DAWG/rdflib/subqueryagg1.rq
new file mode 100644
index 00000000..959188d7
--- /dev/null
+++ b/test/DAWG/rdflib/subqueryagg1.rq
@@ -0,0 +1,7 @@
+PREFIX : <http://people.example/>
+
+SELECT ?y (MIN(?name) as ?minName)
+WHERE {
+  :alice :knows ?y .
+  ?y :name ?name .
+} GROUP BY ?y