Skip to content

Commit

Permalink
GH-1978 fix handling of empty results in grouping/aggregate
Browse files Browse the repository at this point in the history 
- modified and extended unit tests
- disabled incorrect W3C conformance test per discussion on GH-1978
- refactored testsuite setup a bit to make future extension easier
- added two simple test cases in manifest tests
  • Loading branch information
@abrokenjester
abrokenjester committed Mar 12, 2020
1 parent c2c7a52 commit 1767cee
Show file tree
Hide file tree
Showing 17 changed files with 294 additions and 117 deletions.
63 changes: 63 additions & 0 deletions ...parql/src/test/java/org/eclipse/rdf4j/query/parser/sparql/manifest/SPARQL12QueryTest.java
View file
@@ -0,0 +1,63 @@
/*******************************************************************************
* Copyright (c) 2020 Eclipse RDF4J contributors.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Distribution License v1.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/org/documents/edl-v10.php.
*******************************************************************************/
package org.eclipse.rdf4j.query.parser.sparql.manifest;

import java.net.URL;

import org.eclipse.rdf4j.query.Dataset;
import org.eclipse.rdf4j.repository.Repository;
import org.eclipse.rdf4j.repository.dataset.DatasetRepository;
import org.eclipse.rdf4j.repository.sail.SailRepository;
import org.eclipse.rdf4j.sail.memory.MemoryStore;

import junit.framework.Test;

/**
* SPARQL Query tests using testcase from the SPARQL 1.2 Working Group
*
* @author Jeen Broekstra
*
*/
public class SPARQL12QueryTest extends SPARQLQueryTest {

public static Test suite() throws Exception {
URL manifestUrl = SPARQL11ManifestTest.class.getResource("/testcases-sparql-1.2/manifest-all.ttl");

return SPARQL11ManifestTest.suite(new Factory() {

@Override
public SPARQL12QueryTest createSPARQLQueryTest(String testURI, String name, String queryFileURL,
String resultFileURL, Dataset dataSet, boolean laxCardinality) {
return createSPARQLQueryTest(testURI, name, queryFileURL, resultFileURL, dataSet, laxCardinality,
false);
}

@Override
public SPARQL12QueryTest createSPARQLQueryTest(String testURI, String name, String queryFileURL,
String resultFileURL, Dataset dataSet, boolean laxCardinality, boolean checkOrder) {
return new SPARQL12QueryTest(testURI, name, queryFileURL, resultFileURL, dataSet,
laxCardinality, checkOrder);
}
}, manifestUrl.toString(), false);
}

protected SPARQL12QueryTest(String testURI, String name, String queryFileURL, String resultFileURL,
Dataset dataSet, boolean laxCardinality, String... ignoredTests) {
this(testURI, name, queryFileURL, resultFileURL, dataSet, laxCardinality, false, ignoredTests);
}

protected SPARQL12QueryTest(String testURI, String name, String queryFileURL, String resultFileURL,
Dataset dataSet, boolean laxCardinality, boolean checkOrder, String... ignoredTests) {
super(testURI, name, queryFileURL, resultFileURL, dataSet, laxCardinality, checkOrder, ignoredTests);
}

@Override
protected Repository newRepository() {
return new DatasetRepository(new SailRepository(new MemoryStore()));
}
}
19 changes: 14 additions & 5 deletions ...est/java/org/eclipse/rdf4j/query/parser/sparql/manifest/W3CApprovedSPARQL11QueryTest.java
View file
Expand Up @@ -7,27 +7,31 @@
*******************************************************************************/
package org.eclipse.rdf4j.query.parser.sparql.manifest;

import junit.framework.Test;
import java.net.URL;

import org.eclipse.rdf4j.query.Dataset;
import org.eclipse.rdf4j.query.parser.sparql.manifest.SPARQL11ManifestTest;
import org.eclipse.rdf4j.query.parser.sparql.manifest.SPARQLQueryTest;
import org.eclipse.rdf4j.repository.Repository;
import org.eclipse.rdf4j.repository.dataset.DatasetRepository;
import org.eclipse.rdf4j.repository.sail.SailRepository;
import org.eclipse.rdf4j.sail.memory.MemoryStore;

import junit.framework.Test;

public class W3CApprovedSPARQL11QueryTest extends SPARQLQueryTest {

public static Test suite() throws Exception {
URL manifestUrl = SPARQL11ManifestTest.class.getResource("/testcases-sparql-1.1-w3c/manifest-all.ttl");

return SPARQL11ManifestTest.suite(new Factory() {

@Override
public W3CApprovedSPARQL11QueryTest createSPARQLQueryTest(String testURI, String name, String queryFileURL,
String resultFileURL, Dataset dataSet, boolean laxCardinality) {
return createSPARQLQueryTest(testURI, name, queryFileURL, resultFileURL, dataSet, laxCardinality,
false);
}

@Override
public W3CApprovedSPARQL11QueryTest createSPARQLQueryTest(String testURI, String name, String queryFileURL,
String resultFileURL, Dataset dataSet, boolean laxCardinality, boolean checkOrder) {
String[] ignoredTests = {
Expand All @@ -38,12 +42,16 @@ public W3CApprovedSPARQL11QueryTest createSPARQLQueryTest(String testURI, String
// http://lists.w3.org/Archives/Public/public-sparql-dev/2013AprJun/0006.html
"STRLANG TypeErrors",
// known issue: SES-937
"sq03 - Subquery within graph pattern, graph variable is not bound" };
"sq03 - Subquery within graph pattern, graph variable is not bound",
// test case is incorrect wrt SPARQL 1.1 spec, see https://github.com/eclipse/rdf4j/issues/1978
// "agg empty group",
// "Aggregate over empty group resulting in a row with unbound variables"
};

return new W3CApprovedSPARQL11QueryTest(testURI, name, queryFileURL, resultFileURL, dataSet,
laxCardinality, checkOrder, ignoredTests);
}
}, true, true, false, "service");
}, manifestUrl.toString(), true, "service");
}

protected W3CApprovedSPARQL11QueryTest(String testURI, String name, String queryFileURL, String resultFileURL,
Expand All @@ -56,6 +64,7 @@ protected W3CApprovedSPARQL11QueryTest(String testURI, String name, String query
super(testURI, name, queryFileURL, resultFileURL, dataSet, laxCardinality, checkOrder, ignoredTests);
}

@Override
protected Repository newRepository() {
return new DatasetRepository(new SailRepository(new MemoryStore()));
}
Expand Down
11 changes: 6 additions & 5 deletions ...st/java/org/eclipse/rdf4j/query/parser/sparql/manifest/W3CApprovedSPARQL11UpdateTest.java
View file
Expand Up @@ -7,16 +7,13 @@
*******************************************************************************/
package org.eclipse.rdf4j.query.parser.sparql.manifest;

import java.net.URL;
import java.util.Map;

import org.eclipse.rdf4j.model.IRI;
import org.eclipse.rdf4j.query.parser.sparql.manifest.SPARQL11ManifestTest;
import org.eclipse.rdf4j.query.parser.sparql.manifest.SPARQLUpdateConformanceTest;
import org.eclipse.rdf4j.repository.Repository;
import org.eclipse.rdf4j.repository.contextaware.ContextAwareRepository;
import org.eclipse.rdf4j.repository.sail.SailRepository;
import org.eclipse.rdf4j.sail.memory.MemoryStore;
import junit.framework.Test;

import junit.framework.Test;

Expand All @@ -31,16 +28,20 @@ public W3CApprovedSPARQL11UpdateTest(String testURI, String name, String request
}

public static Test suite() throws Exception {

URL manifestUrl = SPARQL11ManifestTest.class.getResource("/testcases-sparql-1.1-w3c/manifest-all.ttl");

return SPARQL11ManifestTest.suite(new Factory() {

@Override
public W3CApprovedSPARQL11UpdateTest createSPARQLUpdateConformanceTest(String testURI, String name,
String requestFile, IRI defaultGraphURI, Map<String, IRI> inputNamedGraphs,
IRI resultDefaultGraphURI, Map<String, IRI> resultNamedGraphs) {
return new W3CApprovedSPARQL11UpdateTest(testURI, name, requestFile, defaultGraphURI, inputNamedGraphs,
resultDefaultGraphURI, resultNamedGraphs);
}

}, true, true, false);
}, manifestUrl.toString(), true);
}

@Override
Expand Down
9 changes: 8 additions & 1 deletion ...e/sparql/src/test/java/org/eclipse/rdf4j/sail/federation/FederationSPARQL11QueryTest.java
View file
Expand Up @@ -7,6 +7,8 @@
*******************************************************************************/
package org.eclipse.rdf4j.sail.federation;

import java.net.URL;

import org.eclipse.rdf4j.query.Dataset;
import org.eclipse.rdf4j.query.parser.sparql.manifest.SPARQL11ManifestTest;
import org.eclipse.rdf4j.query.parser.sparql.manifest.SPARQLQueryTest;
Expand All @@ -20,14 +22,19 @@
public class FederationSPARQL11QueryTest extends SPARQLQueryTest {

public static Test suite() throws Exception {

URL manifestUrl = SPARQL11ManifestTest.class.getResource("/testcases-sparql-1.1-w3c/manifest-all.ttl");

return SPARQL11ManifestTest.suite(new Factory() {

@Override
public SPARQLQueryTest createSPARQLQueryTest(String testURI, String name, String queryFileURL,
String resultFileURL, Dataset dataSet, boolean laxCardinality) {
return new FederationSPARQL11QueryTest(testURI, name, queryFileURL, resultFileURL, dataSet,
laxCardinality);
}

@Override
public SPARQLQueryTest createSPARQLQueryTest(String testURI, String name, String queryFileURL,
String resultFileURL, Dataset dataSet, boolean laxCardinality, boolean checkOrder) {
String[] ignoredTests = {
Expand All @@ -44,7 +51,7 @@ public SPARQLQueryTest createSPARQLQueryTest(String testURI, String name, String
laxCardinality, checkOrder, ignoredTests);
}
// skip 'service' tests for now since they require presence of remote sparql endpoints.
}, true, true, false, "service");
}, manifestUrl.toString(), true, "service");
}

public FederationSPARQL11QueryTest(String testURI, String name, String queryFileURL, String resultFileURL,
Expand Down
7 changes: 4 additions & 3 deletions compliance/sparql/src/test/java/org/eclipse/rdf4j/sail/nativerdf/NativeSPARQLQueryTest.java
View file
Expand Up @@ -9,6 +9,7 @@

import java.io.File;
import java.io.IOException;
import java.net.URL;

import org.eclipse.rdf4j.common.io.FileUtil;
import org.eclipse.rdf4j.query.Dataset;
Expand All @@ -17,15 +18,15 @@
import org.eclipse.rdf4j.repository.Repository;
import org.eclipse.rdf4j.repository.dataset.DatasetRepository;
import org.eclipse.rdf4j.repository.sail.SailRepository;
import org.eclipse.rdf4j.sail.nativerdf.NativeStore;

import junit.framework.Test;

public class NativeSPARQLQueryTest extends SPARQLQueryTest {

public static Test suite() throws Exception {
return SPARQL11ManifestTest.suite(new Factory() {
URL manifestUrl = SPARQL11ManifestTest.class.getResource("/testcases-sparql-1.1-w3c/manifest-all.ttl");

return SPARQL11ManifestTest.suite(new Factory() {
@Override
public NativeSPARQLQueryTest createSPARQLQueryTest(String testURI, String name, String queryFileURL,
String resultFileURL, Dataset dataSet, boolean laxCardinality) {
Expand All @@ -50,7 +51,7 @@ public NativeSPARQLQueryTest createSPARQLQueryTest(String testURI, String name,
}
// skip 'service' tests because it requires the test rig to start up
// a remote endpoint
}, true, true, false, "service");
}, manifestUrl.toString(), true, "service");
}

private File dataDir;
Expand Down
14 changes: 8 additions & 6 deletions ...tion/src/main/java/org/eclipse/rdf4j/query/algebra/evaluation/iterator/GroupIterator.java
View file
Expand Up @@ -197,12 +197,14 @@ private Collection<Entry> buildEntries() throws QueryEvaluationException {
Map<Key, Entry> entries = new LinkedHashMap<>();

if (!iter.hasNext()) {
// no solutions, but if aggregates are present we still need to process them to produce a
// zero-result.
final Entry entry = new Entry(null);
if (!entry.getAggregates().isEmpty()) {
entry.addSolution(EmptyBindingSet.getInstance());
entries.put(new Key(EmptyBindingSet.getInstance()), entry);
// no solutions, but if we are not explicitly grouping and aggregates are present,
// we still need to process them to produce a zero-result.
if (group.getGroupBindingNames().isEmpty()) {
final Entry entry = new Entry(null);
if (!entry.getAggregates().isEmpty()) {
entry.addSolution(EmptyBindingSet.getInstance());
entries.put(new Key(EmptyBindingSet.getInstance()), entry);
}
}
}

Expand Down
13 changes: 12 additions & 1 deletion .../src/test/java/org/eclipse/rdf4j/query/algebra/evaluation/iterator/GroupIteratorTest.java
View file
Expand Up @@ -71,7 +71,7 @@ public void testAvgEmptySet() throws QueryEvaluationException {
}

@Test
public void testMaxEmptySet() throws QueryEvaluationException {
public void testMaxEmptySet_DefaultGroup() throws QueryEvaluationException {
Group group = new Group(EMPTY_ASSIGNMENT);
group.addGroupElement(new GroupElem("max", new Max(new Var("a"))));
GroupIterator gi = new GroupIterator(evaluator, group, EmptyBindingSet.getInstance());
Expand All @@ -80,6 +80,17 @@ public void testMaxEmptySet() throws QueryEvaluationException {
assertThat(gi.next().size()).isEqualTo(0);
}

@Test
public void testMaxEmptySet_Grouped() throws QueryEvaluationException {
Group group = new Group(EMPTY_ASSIGNMENT);
group.addGroupElement(new GroupElem("max", new Max(new Var("a"))));
group.addGroupBindingName("x"); // we are grouping by variable x

GroupIterator gi = new GroupIterator(evaluator, group, EmptyBindingSet.getInstance());

assertThat(gi.hasNext()).isFalse();
}

@Test
public void testMinEmptySet() throws QueryEvaluationException {
Group group = new Group(EMPTY_ASSIGNMENT);
Expand Down
48 changes: 45 additions & 3 deletions ...es/sparql/src/main/java/org/eclipse/rdf4j/query/parser/sparql/ComplexSPARQLQueryTest.java
View file
Expand Up @@ -1118,12 +1118,26 @@ public void testValuesInOptional() throws Exception {
* See https://github.com/eclipse/rdf4j/issues/1978
*/
@Test
public void testMaxAggregateEmptyResult() throws Exception {
public void testMaxAggregateWithGroupEmptyResult() throws Exception {
String query = "select ?s (max(?o) as ?omax) {\n" +
" ?s ?p ?o .\n" +
" }\n" +
" group by ?s\n";

try (TupleQueryResult result = conn.prepareTupleQuery(query).evaluate()) {
assertThat(result.hasNext()).isFalse();
}
}

/**
* See https://github.com/eclipse/rdf4j/issues/1978
*/
@Test
public void testMaxAggregateWithoutGroupEmptySolution() throws Exception {
String query = "select (max(?o) as ?omax) {\n" +
" ?s ?p ?o .\n" +
" }\n";

try (TupleQueryResult result = conn.prepareTupleQuery(query).evaluate()) {
assertThat(result.next()).isEmpty();
}
Expand All @@ -1133,12 +1147,26 @@ public void testMaxAggregateEmptyResult() throws Exception {
* See https://github.com/eclipse/rdf4j/issues/1978
*/
@Test
public void testMinAggregateEmptyResult() throws Exception {
public void testMinAggregateWithGroupEmptyResult() throws Exception {
String query = "select ?s (min(?o) as ?omin) {\n" +
" ?s ?p ?o .\n" +
" }\n" +
" group by ?s\n";

try (TupleQueryResult result = conn.prepareTupleQuery(query).evaluate()) {
assertThat(result.hasNext()).isFalse();
}
}

/**
* See https://github.com/eclipse/rdf4j/issues/1978
*/
@Test
public void testMinAggregateWithoutGroupEmptySolution() throws Exception {
String query = "select (min(?o) as ?omin) {\n" +
" ?s ?p ?o .\n" +
" }\n";

try (TupleQueryResult result = conn.prepareTupleQuery(query).evaluate()) {
assertThat(result.next()).isEmpty();
}
Expand All @@ -1148,12 +1176,26 @@ public void testMinAggregateEmptyResult() throws Exception {
* See https://github.com/eclipse/rdf4j/issues/1978
*/
@Test
public void testSampleAggregateEmptyResult() throws Exception {
public void testSampleAggregateWithGroupEmptyResult() throws Exception {
String query = "select ?s (sample(?o) as ?osample) {\n" +
" ?s ?p ?o .\n" +
" }\n" +
" group by ?s\n";

try (TupleQueryResult result = conn.prepareTupleQuery(query).evaluate()) {
assertThat(result.hasNext()).isFalse();
}
}

/**
* See https://github.com/eclipse/rdf4j/issues/1978
*/
@Test
public void testSampleAggregateWithoutGroupEmptySolution() throws Exception {
String query = "select (sample(?o) as ?osample) {\n" +
" ?s ?p ?o .\n" +
" }\n";

try (TupleQueryResult result = conn.prepareTupleQuery(query).evaluate()) {
assertThat(result.next()).isEmpty();
}
Expand Down

0 comments on commit 1767cee

Please sign in to comment.