GH-5310 Correctly evaluate constant grouping constants for empty result sets.

[JervenBolleman]

GitHub issue resolved: #GH-5310

Briefly describe the changes proposed in this PR:

The logic to deal with group elements and empty results set was not correct for constant operator values.
This is fixed, plus minor code cleanups.

---

PR Author Checklist (see the contributor guidelines for more details):

• my pull request is self-contained
• I've added tests for the changes I made
• I've applied code formatting (you can use mvn process-resources to format from the command line)
• I've squashed my commits where necessary
• every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change

[JervenBolleman]

@nguyenm100 would you mind reviewing this?

[nguyenm100]

hey @odysa can you help Jervan with the review?

[odysa]

Let's try to test all aggregate functions here.

String query = String.join("\n",
		"PREFIX ex: <ex:>",
		"SELECT",
		"  (MAX(1) AS ?a)",
		"  (MIN(1) AS ?b)",
		"  (AVG(1) AS ?c)",
		"  (COUNT(1) AS ?d)",
		"  (COUNT(DISTINCT 1) AS ?e)",
		"  (COUNT(*) AS ?f)",
		"WHERE {",
		"  ?x a ?z ;",
		"  ex:someProperty ?val .",
		"}"
);
.....

assertTrue(bindings.hasBinding("a")); 
assertTrue(bindings.hasBinding("b")); 
assertTrue(bindings.hasBinding("c"));
assertTrue(bindings.hasBinding("d")); 
assertTrue(bindings.hasBinding("e")); 
assertTrue(bindings.hasBinding("f"));
assertEquals(1, ((Literal) bindings.getBinding("a").getValue()).intValue());
assertEquals(1, ((Literal) bindings.getBinding("b").getValue()).intValue());
assertEquals(1, ((Literal) bindings.getBinding("c").getValue()).intValue());
assertEquals(1, ((Literal) bindings.getBinding("d").getValue()).intValue());
assertEquals(1, ((Literal) bindings.getBinding("e").getValue()).intValue());
assertEquals(0, ((Literal) bindings.getBinding("f").getValue()).intValue());

[JervenBolleman]

I Will include that!

[JervenBolleman]

@odysa
I think this is not right for the empty case, but I am not sure.
" (COUNT(1) AS ?d)" != assertEquals(1, ((Literal) bindings.getBinding("d").getValue()).intValue());

[odysa]

Looks like there is no better way to test AggregateFunction<CountCollector, BindingSet> in Java. Just need to be careful when somebody in the future make a new BindingSet aggregator. (very unlikely?)

[JervenBolleman]

Yeah, in a newer java we could make that a sealed class and we would get compilation time errors.

[odysa]

Does this unblock the previous PR #5344 ?

[JervenBolleman]

Does this unblock the previous PR #5344 ?

No, previous PR#5344 is broken and is not the right solution.

[JervenBolleman]

@odysa @hmottestad this is getting into the specification corners of how aggregates are supposed to behave in the face of the empty result set.

SELECT
(COUNT(1) AS ?d)
WHERE {
  ?s ?p ?o
}

on an empty result set should I believe return '0' not '1'.

SELECT
(MAX(1) AS ?d)
WHERE {
  ?s ?p ?o
}

I believe should '1', other stores return an UNDEF/null binding here.

If it should be an UNDEF the logic in the empty case can be simplified. To return a zero in the case of the count operators and a UNDEF for all other cases.

[odysa]

@JervenBolleman I agree count should return 0 in this case.