JPOX must throw JDOUserException for a query using a non grouping expr w/o aggregates in the HAVING clause.

• Key: JDO-245
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Andy Jefferson
• Reporter: Michael Watzek
• Component/s: tck2

Description

The test case Having fails for the query below. Query compilation is expected to throw a JDOUserException because the having clause contains field firstname which is not an aggregate and is not part of the grouping clause.

14:22:53,437 (main) DEBUG [org.apache.jdo.tck] - Compiling API query: SELECT department, SUM(salary) FROM org.apache.jdo.tck.pc.company.Employee GROUP BY department HAVING firstname == 'emp1First'
14:22:53,453 (main) DEBUG [org.apache.jdo.tck] - Query compilation must throw JDOUserException: null
14:22:53,453 (main) INFO [org.apache.jdo.tck] - Exception during setUp or runtest:
junit.framework.AssertionFailedError: Assertion A14.6.10-2 (Having) failed:
Query compilation must throw JDOUserException: null
at junit.framework.Assert.fail(Assert.java:47)
at org.apache.jdo.tck.JDO_Test.fail(JDO_Test.java:546)
at org.apache.jdo.tck.query.QueryTest.compile(QueryTest.java:915)
at org.apache.jdo.tck.query.QueryTest.compile(QueryTest.java:878)
at org.apache.jdo.tck.query.QueryTest.compileAPIQuery(QueryTest.java:793)
at org.apache.jdo.tck.query.result.Having.testNegative(Having.java:120)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:324)
at junit.framework.TestCase.runTest(TestCase.java:154)
at org.apache.jdo.tck.JDO_Test.runBare(JDO_Test.java:204)
at junit.framework.TestResult$1.protect(TestResult.java:106)
at junit.framework.TestResult.runProtected(TestResult.java:124)
at junit.framework.TestResult.run(TestResult.java:109)
at junit.framework.TestCase.run(TestCase.java:118)
at junit.framework.TestSuite.runTest(TestSuite.java:208)
at junit.framework.TestSuite.run(TestSuite.java:203)
at junit.framework.TestSuite.runTest(TestSuite.java:208)
at junit.framework.TestSuite.run(TestSuite.java:203)
at junit.textui.TestRunner.doRun(TestRunner.java:116)
at junit.textui.TestRunner.doRun(TestRunner.java:109)
at org.apache.jdo.tck.util.BatchTestRunner.start(BatchTestRunner.java:120)
at org.apache.jdo.tck.util.BatchTestRunner.main(BatchTestRunner.java:95)

Comments

[Andy Jefferson]

I look at the test Having.testNegative and the query is now
SELECT department, SUM(salary) FROM Employee GROUP BY department HAVING firstname
What is this exactly supposed to test ?
Is it testing that the having clause is a boolean expression (which it clearly isn't) ?
Is it testing that a having field is not in the grouping ?

The query in the original post had a having clause of "firstname == 'emp1first'". Did someone change this for a reason ?

[Michael Bouschen]

The negative test is only testing whether the JDO implementation throws an exception for the invalid HAVING clause. It does not matter which of the two errors (you descibed above) is reported.

This is the only negative test for HAVING we have so far and I wanted to have a query that is invalid w/o a doubt. I can change it back to "firstname == 'emp1first'", if you prefer the original version.

[Andy Jefferson]

Thanks for your reply Michael. Let's leave the test as it is ;-)
Now fixed in JPOX CVS - builds dated 04/01/2006 onwards have this.

[Craig Russell]

There is only one thing wrong with this query: the HAVING clause is not a boolean expression.

It's ok to have SUM(salary) in the SELECT clause because you can SELECT terms that are either in the GROUPING clause or are aggregates.

A14.6.10-1 [When grouping is specified, each result expression must be one of:
an expression contained in the grouping expression; or,
an aggregate expression evaluated once per group.]

[Craig Russell]

Actually, I'll correct myself. The SUM(salary) is not correct because salary is not a field in Employee. Again.

I think this should be changed to AVG(weeklyhours) to be valid. Then the test case is only testing the HAVING error.

[Craig Russell]

Yet another comment. The title of this JIRA is the HAVING clause containing fields that are not part of the result clause. Actually, it's legal for any aggregate expression to be in the HAVING clause regardless of whether it is in the result.

So maybe we need another positive test for HAVING that has an expression that isn't contained in the SELECT clause.
e.g. SELECT department, AVG(weeklyhours) FROM Employee GROUP BY department HAVING COUNT(personid) > 1

And another negative test for HAVING that uses a term that's not an aggregate.
SELECT department, AVG(weeklyhours) FROM Employee GROUP BY department HAVING middlename != NULL

[Michael Bouschen]

A lot of comments, I'll try to give answers.

CLR: There is only one thing wrong with this query: the HAVING clause is not a boolean expression. It's ok to have SUM(salary) in the SELECT clause because you can SELECT terms that are either in the GROUPING clause or are aggregates.

MBO: I think the HAVING clause "HAVING firstname" is invalid for two reasons: it is not a boolean expression and uses a field firstname w/o aggregate that is not part of the grouping. These are the two errors Andy and I were referring to.

CLR: I think this should be changed to AVG(weeklyhours) to be valid. Then the test case is only testing the HAVING error.

MBO: I already changed the negative test to
  SELECT department, AVG(weeklyhours) FROM Employee GROUP BY department HAVING firstname
when fixing the positive test of class Having (see JDO-244).

CLR: Yet another comment. The title of this JIRA is the HAVING clause containing fields that are not part of the result clause. Actually, it's legal for any aggregate expression to be in the HAVING clause regardless of whether it is in the result.

MBO: I think the title of this JIRA is misleading. It should talk about a missing exception for an invalid HAVING clause and should not mention the result clause at all. Maybe the test class Having should be moved from package result to a different package (e.g. jdoql).

CLR: So maybe we need another positive test for HAVING that has an expression that isn't contained in the SELECT clause.
e.g. SELECT department, AVG(weeklyhours) FROM Employee GROUP BY department HAVING COUNT(personid) > 1

MBO: Yes, good idea. I will add this query.

CLR: And another negative test for HAVING that uses a term that's not an aggregate.
SELECT department, AVG(weeklyhours) FROM Employee GROUP BY department HAVING middlename != NULL

MBO: OK, then we are back to the original negative query which had a HAVING clause: HAVING firstname = 'emp1first'. But I can add this, too.

[Michael Bouschen]

I added a new negative test query as discussed above:
  SELECT department, AVG(weeklyhours) FROM Employee GROUP BY department HAVING firstname == 'emp1First'

The HAVING clause is not valid, because it uses the field firstname w/o aggregate and firstname is not used for grouping. JPOX currently does not catch the error.

[Andy Jefferson]

Oh, if you insist. Fixed in JPOX CVS - builds dated 06/01/2006 or later.