Misleading replacement of commas with cross joins

Description

There was a new feature implemented in 3.5.0-Beta-2: HHH-1480, as a result, for many dialects, the comma operator "," was replaced with "cross join". As it's already obvious from the feature's description, this changes join precedence. If the query contains outer joins (left or right) this replacement changes the results (because outer join precedence is important).
This might not be interpreted as a bug if you know what SQL operator will be generated when you place a comma in HQL, but it's misleading if you don't. A HQL query like:

is not equivalent with the SQL query:

because the actual SQL query is:

The first SQL query would make a left join on B and C and then a cross join with the result and A (and this is what we should expect). The second SQL query makes a cross join on A and B and then a left join with the result and C (this is not expected since we had a comma in HQL).

I understand the issues presented in HHH-1480, but they should be expected, i.e. a developer SHOULD expect a faliure from a query like

because "a" is not visible in the join. Hibernate should not use a cross join behind the scenes to allow such a query at the expense of providing misleading results for others.

Maybe turn this into a feature/improvement or add another to allow explicit comma or croos join in HQL, or support parenthesis in HQL.

The only workaround I can see for now is to re-order the query like this:

Environment

Hibernate 3.5.3
Ms SQL Server 2005

Activity

Show:
Vlad Patras
January 11, 2011, 1:05 PM

Yes, it still an issue in 3.6.0

This issue is probably due to the not so obvious difference between comma and cross join SQL operators (as of SQL-99 standard).
On many forums and even books it's stated that they are functionally equivalent and it's just a matter of preference or what the DBE supports.
But they are NOT equivalent, for example consider the following SQL:

When running the above SQL in an SQL-99 compliant DBE (such as MySQL 5 or MS SQL Server 2005), Q1 and Q2 will return different results (Q1> 1 null null, Q2> null null null). The only difference between Q1 and Q2 is comma being replaced with CROSS JOIN, thus the two operators are not equivalent.

From the hibernate source code:

org.hibernate.dialect.Dialect.java

It is obvious that the developer is not aware that CROSS JOIN is different from comma. First, the method is named getCrossJoinSeparator even though it refers to what a comma in HQL is going to be replaced with, and then there is the comment that says "Typically this will be either <tt> cross join </tt> or <tt>, </tt>".

This functionality was implemented to fix HHH-1480. But that is not an actual fix, it's a hack.
Basicly says "JOIN and comma have different precedence in SQL but they have the same precedence in HQL".
Instead of fixing the problem, the solution was "Oh, we will just change the meaning of comma in HQL to be a CROSS JOIN so it will have the same precedence as other JOINs".
Changing the meaning of comma changes the results returned by HQL queries. All this happens silently, it's not even specified in the documentation. If an application does not have comprehensive unit testing this is likely to go by unnoticed.

Gail Badner
January 17, 2011, 10:41 PM

Please update org.hibernate.test.hql.HQLTest to reproduce your issue.
Thanks,
Gail

Vlad Patras
January 18, 2011, 3:07 PM
Edited

When was implemented it is obvious that org.hibernate.test.hql.HQLTest would have failed.
So the developer added public static boolean REGRESSION_STYLE_CROSS_JOINS = false; to org.hibernate.hql.ast.SqlGenerator.
This is set to true in prepareTest and to false in cleanupTest.
If you want HQLTest to fail just set REGRESSION_STYLE_CROSS_JOINS to false again, it's only purpose is to make HQLTest pass.

Even the test case considers "cross join" and comma to be equivalent by ignoring the change (it tells the query generator to use commas again just for testing purposes). There is no test that actually checks if the generated sql returns the same thing after the change.

For this purpose I modified org.hibernate.test.orphan.OrphanTest (it has suitable entities).
The important bit is:

It is obvious from the test that when creating sql the old way (REGRESSION_STYLE_CROSS_JOINS is true) everything works as expected. But when replacing commas with "cross join" (REGRESSION_STYLE_CROSS_JOINS is false), the query dosn't return the expected results and even crashes on HyperDB 2.

Note that a patch with the changes is attached.

Steve Ebersole
April 14, 2011, 10:58 PM

Please stop telling me what I am obviously aware of and what I am obviously not aware of etc, thanks.

I did what is workable given the current codebase. Reordering the joins is the proper thing to do, but thats not feasible in the current codebase.

Ben Pepper
August 19, 2013, 7:23 PM

I was wondering about the status of this bug. It appears to be fixed on this "antlr-rework" branch, but I can see no documentation anywhere about what this branch is. For now I've overridden the getCrossJoinSeparator() method in the dialect, but this seems more of a temporary hack, especially if I actually want "cross join" in some place in the future.

Fixed

Assignee

Unassigned

Reporter

Vlad Patras

Fix versions

Labels

None

backPortable

None

Suitable for new contributors

None

Requires Release Note

None

Pull Request

None

backportDecision

None

Priority

Major
Configure