clean up org.hibernate.query package

[gavinking]

Since Query lives in this package, this is one of the two most "public user-facing API" packages in Hibernate.

Currently, in H6, it has accumulated quite a lot of cruft:

• some of which isn't API (e.g. NavigablePath,TreatedNavigablePath, EntityIdentifierNavigablePath, and QueryLogging),
• some of which looks sorta experimental (e.g. JpaTuple, which declares no operations, and IntervalType which is an enum with one member), and
• some of which probably doesn't belong in this package at all (IntervalType again, probably Limit, maybe ImmutableEntityUpdateQueryHandlingMode).

There are also a bunch of enums which represent concepts which are sort-of common across HQL and criteria, and I don't hate them being there, but they're still not really user-facing APIs and perhaps we should find a better place for them too.

Overall the package just looks like a bucket of stuff that we didn't know where else to put.

I think it's very important that we clean this up before H6 goes out.

(Well, either that, or we move Query back to org.hibernate.)

[sebersole]

some of which probably doesn't belong in this package at all (IntervalType again, probably Limit, maybe ImmutableEntityUpdateQueryHandlingMode).

• not sure of the api/spi intention of IntervalType.
• Limit imo does belong here - it is a valid part of a SQM/Criteria tree, so users might want to reference it if they process the Criteria as SQM tree in their code.
• ImmutableEntityUpdateQueryHandlingMode is also "user facing", but I don't think user code would ever reference it directly. Generally they configure it by name. And it is not exposed anywhere other than spi/internal.

bunch of enums ... not really user-facing APIs and perhaps we should find a better place for them

Hard to say without specifics. Bit open-ended. Quick notes on some specific ones -

• FetchClauseType is not really even Query based, is it @beikov ? Seems more SQL related
• NullOrdering / NullPrecedence - @beikov why did you add NullOrdering when we already had NullPrecedence? What is the intent and/or gain?

NavigablePath

IMO should be API. Under the premise that users might in fact reference these SQM / Criteria nodes in their code.

DotIdentifierSequence should go somewhere else as it is implemented outside of queries as well (NavigableRole is such a sequence). But putting it in o.h doesn't make much sense either. So ... 🤷‍♂️

JpaTuple

Probably could go away. I generally try to sub-type all JPA contracts as JpaX, but not important here since it does not define any additional capabilities (yet?). Though maybe some forEach-style methods make sense here..

Well, either that, or we move Query back to org.hibernate.

Nothing says we cannot do both. It was a mistake to move Query itself to the new package in hindsight. What has stopped me from reversing that has been the fact that we deprecated the top level one. Not sure it is a great idea to undo a deprecation that has directed users to something else. Otherwise you get into situation where in earlier version we've told users to move from o.h.Query to o.h.query.Query and now we tell them to go back to the original. If I, as a user, made the move to o.h.query.Query based on the deprecation warnings, I'd be a little upset at having to move back to what I already had before. Not saying that should unequivocally stop us, but definitely something to consider.

Regarding the rest...

The logging objects are interesting. I had starting putting them in API mainly on the premise that users could see the associated logger categories. But in retrospect, that is better handled via documentation (User Guide or something like design/logger_id_ranges.adoc)

ConstructorEntityArgumentMode is not yet implemented, but will be - just ran out of time for 6.0

[gavinking]

Nothing says we cannot do both. It was a mistake to move Query itself to the new package in hindsight. What has stopped me from reversing that has been the fact that we deprecated the top level one.

FWIW if it were up to me I would do it. And now is sort of the perfect moment.

If I, as a user, made the move to o.h.query.Query based on the deprecation warnings, I'd be a little upset at having to move back to what I already had before. Not saying that should unequivocally stop us, but definitely something to consider.

I know, but we have a reasonable justification: "well, we were finally able to remove the old deprecated one".

[beikov]

FetchClauseType is not really even Query based, is it @beikov ? Seems more SQL related

It is both, HQL/Criteria and SQL related used in SqmQueryPart and QueryPart.

NullOrdering / NullPrecedence - @beikov why did you add NullOrdering when we already had NullPrecedence? What is the intent and/or gain?

NullPrecedence describes a user requested or explicit null ordering. NullOrdering describes how nulls are ordered by a database in general and also "by default". Usually, null is the greatest value i.e. comes last when asc and first when desc. Some databases sort null always first/last irrespective of the sort asc/desc. Dialects tell us the default null ordering of the database so we can omit the clause if it's unnecessary.

IntervalType

It's something that is used in Dialect and TypeConfiguration, not sure where else to put it. Anyway, it has only 1 member right now because I didn't have time yet to add other resolutions.

[sebersole]

NullPrecedence describes a user requested or explicit null ordering

I'm not following the difference you are trying to make here. How is something like this "explicit" if the user did not request it?

And still not following why you thought these should be different. iiuc your argument is because they are used in different, but same ways? I mean null ordering is null ordering. Sure you created NullOrdering with more values, but why is that better than duplicating an enum that describes the same exact thing rather than simply adding those 2 new values to the already existing one?

[beikov]

Not sure what else to write. NullPrecedence is an enum for the allowed null ordering in the order by clause. Whereas NullOrdering (the name is debatable) is an enum that contains entries for all default orderings that databases use e.g. SMALLEST meaning that null is the smallest value i.e. for asc null comes first and for desc last.

[gavinking]

Another thing I just ran into: we have two very-similar-looking SynchronizableQuery interfaces in different packages, one for native queries, in org.hibernate.query, and one for procedure calls, in org.hibernate. It seems to me that we don't need both.

[sebersole]

Another thing I just ran into: we have two very-similar-looking SynchronizableQuery interfaces in different packages, one for native queries, in org.hibernate.query, and one for procedure calls, in org.hibernate. It seems to me that we don't need both.

I think that I just did a bad job there when moving the query packages around. +1 for consolidating them

[gavinking]

Almost everything here was fixed already, and the package looks a lot better in H6.

The one remaining thing I notice is that we still have @Incubating interface JpaTuple with no operations.

Since it's @Incubating, we could still remove it if it's not needed.

@sebersole?