[#519] Introduce specialized expression types for easier JPA Criteria querying

[beikov]

Fixes https://github.com/jakartaee/persistence/issues/519

[gavinking]

Sorry, @beikov, finally found time to look at this. A couple of comments which are really questions:

1. It seems to me that we should either clearly maintain the distinction between Predicate and BooleanExpression, in which case logical operators don't apply to BooleanExpressions (or, if they do, they return BooleanExpression instead of Predicate) or simply drop the distinction. [Today, JPQL does not allow the use of logical operators with boolean expressions.]
2. If we bother having ComparableExpression at all, then I would have expected NumberExpression to extend it. But perhaps this was just an oversight?
3. I expected to find overloads of the get() operation which e.g. accept NumberAttribute and return NumberExpression. Did I just miss something?
4. I think we need to weigh this carefully. This really does expand the API surface by a lot. It might be worth it, but let's think it through.

[beikov]

It seems to me that we should either clearly maintain the distinction between Predicate and BooleanExpression, in which case logical operators don't apply to BooleanExpressions (or, if they do, they return BooleanExpression instead of Predicate) or simply drop the distinction. [Today, JPQL does not allow the use of logical operators with boolean expressions.]

Predicate extends BooleanExpression, but I think I'd prefer dropping the distinction, though I have no idea what that means in terms of spec changes. I'd prefer to return Predicate from the new methods and also consider making BooleanPath extend Predicate. Let me know what you think about this.

If we bother having ComparableExpression at all, then I would have expected NumberExpression to extend it. But perhaps this was just an oversight?

Unfortunately, Number does not extend Comparable, so it's not possible to unify the two. The Comparable stuff is mainly about being a base type for the temporal, boolean and string types. I suppose that for the same reason there are two method styles in CriteriaBuilder e.g. gt and greaterThan.

I expected to find overloads of the get() operation which e.g. accept NumberAttribute and return NumberExpression. Did I just miss something?

That's all in Path see <Y extends Number> NumberPath<Y> get(NumberSingularAttribute<? super X, Y> attribute);

I think we need to weigh this carefully. This really does expand the API surface by a lot. It might be worth it, but let's think it through.

It does add a lot of API surface, which is why I asked if you agree this is a good idea for making JPA Criteria more attractive. I wanted to highlight that this approach is similar to what QueryDSL does, so we might be able to learn from their design: https://github.com/querydsl/querydsl/tree/master/querydsl-core/src/main/java/com/querydsl/core/types/dsl

[gavinking]

@beikov

Predicate extends BooleanExpression, but I think I'd prefer dropping the distinction

I think that makes sense. It might actually be a mistake that Predicate is an Expression<Boolean>, but since it already is (ever since JPA 2), my prejudice is that we shouldn't need both Predicate and BooleanExpression.

Unfortunately, Number does not extend Comparable, so it's not possible to unify the two.

I see, OK.

The Comparable stuff is mainly about being a base type for the temporal, boolean and string types.

I'm not convinced that's worth it. I don't think I agree that Boolean should even be treated as comparable (no matter what Java thinks). And having ComparableExpression just to abstract over TemporalExpression and StringExpression doesn't seem worthwhile to me. I would vote to drop it.

That's all in Path

OK 👍

It does add a lot of API surface, which is why I asked if you agree this is a good idea for making JPA Criteria more attractive.

I'm not against this change; actually I'm in favor of it.

I just like to make the "cons" explicit.

[gavinking]

And having ComparableExpression just to abstract over TemporalExpression and StringExpression doesn't seem worthwhile to me. I would vote to drop it.

Well, actually, looking at that again, I think you still could make NumberExpression extend ComparableExpression. Just remove references to the type Comparable from ComparableExpression.

[beikov]

I think that makes sense. It might actually be a mistake that Predicate is an Expression, but since it already is (ever since JPA 2), my prejudice is that we shouldn't need both Predicate and BooleanExpression.

So you want to drop BooleanExpression and return Predicate in the new methods instead? I'm fine with that. Final words from @lukasj?

I'm not convinced that's worth it. I don't think I agree that Boolean should even be treated as comparable (no matter what Java thinks). And having ComparableExpression just to abstract over TemporalExpression and StringExpression doesn't seem worthwhile to me. I would vote to drop it.

There are a few types that JPA supports which are also Comparable for which I would consider the ComparableExpression etc. to be useful, e.g. UUID, java.util.Date, java.util.Calendar, java.lang.Enum. So in the end, the static JPA metamodel would generate ComparableSingularAttribute for attributes of these types.

Well, actually, looking at that again, I think you still could make NumberExpression extend ComparableExpression. Just remove references to the type Comparable from ComparableExpression.

I would prefer not to do that, but if @lukasj thinks the same way then I'd be open for it.

[gavinking]

So you want to drop BooleanExpression and return Predicate in the new methods instead?

Yeah, I think so, assuming @lukasj agrees.

There are a few types that JPA supports which are also Comparablefor which I would consider the ComparableExpression etc. to be useful, e.g. UUID, java.util.Date, java.util.Calendar, java.lang.Enum.

OK, so:

1. UUID doesn't have a meaningful order, and
2. java.util.Date, java.util.Calendar are now officially deprecated in JPA, but
3. I agree that there are some enum types which do have a well-defined order, even though I argued that they're the exception here.

So, sure, point taken, it would be nice to be able to compare enums.

So, fine, ComparableExpression survives.

I would prefer not to do that?

Why? Numbers are in principle comparable. (I don't think we need to consider complex numbers here.)

And all the known subtypes of Number, including Double, Long, Byte, BigInteger, and BigDecimal all implement Comparable.

The only reason Number itself doesn't implement Comparable is because it's not a generic type.

Actually, the solution might be even simpler. Just add Comparable<X> as an upper bound on the type parameter of NumberExpression. Does that work?

[beikov]

Actually, the solution might be even simpler. Just add Comparable as an upper bound on the type parameter of NumberExpression. Does that work? @beikov How about changing this to: public interface NumberExpression<X extends Number&Comparable<X>> extends ComparableExpression<X> {

That could work, yes, but it brings up the question why CriteriaBuilder has two method flavors for the relational stuff for Number vs. Comparable. I can only assume it has to do with the way Javac does type inference when you have <T extends Number> Expression<T> operation(T... expressions) i.e. if you pass arguments of the same type, T will have that type and otherwise it will default to Number. But maybe @lukasj can shed some light on this.

[gavinking]

That could work, yes, but it brings up the question why CriteriaBuilder has two method flavors for the relational stuff for Number vs. Comparable.

Yeah that's what I just commented on the code, before I saw your reply:

it's supposed to let you do inter-numeric-type comparisons, that is, compare a Double to an Integer, for example.

[lukasj]

wrt Predicate vs BooleanExpression - should this be designed from scratch, my preference would be to deprecate everything related to Predicate and promote usage of BooleanExpression instead because 1) Predicate itself sort of conflicts with j.u.function.Predicate and 2) BooleanExpression is more consistent with the rest of Criteria API. In any case, I agree that we don't need both of them

[gavinking]

I don't think I agree that Boolean should even be treated as comparable (no matter what Java thinks).

Actually this statement, which I made a long time ago, without thinking too hard, is completely wrong.

We absolutely want to be able to include a boolean in an order by. Fortunately Boolean is Comparable in Java, so there's no issue there.

[gavinking]

We absolutely want to be able to include a boolean in an order by. Fortunately Boolean is Comparable in Java, so there's no issue there.

And for a similar reason we do want to be able to sort by UUID.

[jwgmeligmeyling]

I think that makes sense. It might actually be a mistake that Predicate is an Expression, but since it already is (ever since JPA 2), my prejudice is that we shouldn't need both Predicate and BooleanExpression.

If this were to be changed, does that mean that a boolean expression can be used where a predicate is expected in the CNF?

I.e.:

• case when entity.someBoolean then 1 end
• where entity.someBoolean instead of where entity.someBoolean is true
• where someBooleanReturningFunction()

Because otherwise it seems desirable to distinguish between Expression<Boolean>/BooleanExpression and Predicate.

[gavinking]

@jwgmeligmeyling unfortunately operations like where() and having() and CriteriaBuilder.and() and Case.when() are currently typed to accept Expression<Boolean>, so the Predicate interface currently has little value.

This was arguably a mistake in the original design of the criteria API, but it's a mistake we're kinda stuck with.