Add extract() to CriteriaBuilder

[gavinking]

This approach isn't completely typesafe, but it's significantly less complex than a solution which is.

Questions:

• Do we want to buy into the extra complexity required to make this properly typesafe? (DateField, TimeField, and DateTimeField instead of just TemporalField.)
• What package should TemporalField be defined in?

[lukasj]

if there are no objections from others, add the license header so this can be part of 3.1 (by the end of Feb, we have to have the final build etc, so do it rather sooner than later, thx); otherwise it has to wait for the next release.

[dazey3]

What is the usecase for this support? How would this be used and is there a JPQL equivalent?

[lukasj]

@dazey3 see #314

[lukasj]

@gavinking are you going to fix the missing license header today? thx!

[gavinking]

I've added the license header, and finished the Javadoc.

I would still like to know if anyone thinks we should make this more typesafe, so that, for example, you can't try to extract a YEAR from a LocalTime, or a MINUTE from a LocalDate. I lean toward thinking this is unnecessary, but I really want to hear from others.

[AleksNo]

Hello,

personally i prefer stronger type safety if it does not make the API or implementations way more complicated. This is one of the most important features of the criteria API, IMHO. And such features should be strengthened.

Cheers

[beikov]

I would still like to know if anyone thinks we should make this more typesafe

Can you sketch out how the type safety could look like? Since the types like LocalDate, LocalDateTime etc. have no common super type per extractable component groups (date or time parts), I would expect the only type safe way would be to introduce several extractXXX methods to avoid clashing signatures due to JVM raw types.

If that is the only way to do it, I am against it. It complicates everything by a lot and it doesn't help you much type safety wise. I mean how often are you going to change from a DateTime type to a simple Time type which is AFAICT the only case when the type safety would play a role.

[scottmarlow]

@lukasj is this pull request targeted for Persistence 3.1?

Also could we get a RC3 pushed to maven central to ensure that all Persistence 3.1 compatible implementations have access to the latest changes without depending on the Eclipse Maven staging repo?

Thanks!

[lukasj]

@scottmarlow

is this pull request targeted for Persistence 3.1?

no, there is no clear conclusion on this (apart from missing the end of Feb deadline)

Also could we get a RC3 pushed to maven central to ensure that all Persistence 3.1 compatible implementations have access to the latest changes without depending on the Eclipse Maven staging repo?

why exactly do you need the RC3 build when I see you already use the final version in the tck? Note that signatures between RC3 and final build differs

[scottmarlow]

Also could we get a RC3 pushed to maven central to ensure that all Persistence 3.1 compatible implementations have access to the latest changes without depending on the Eclipse Maven staging repo?

why exactly do you need the RC3 build when I see you already use the final version in the tck? Note that signatures between RC3 and final build differs

For other persistence providers that also want to prepare be one of the Persistence 3.1 implementations. Does the RC2 milestone build have all of the SPEC API changes that went into 3.1.0?

[lukasj]

Does the RC2 milestone build have all of the SPEC API changes that went into 3.1.0?

No. Neither does RC1.

[scottmarlow]

Does the RC2 milestone build have all of the SPEC API changes that went into 3.1.0?

No. Neither does RC1.

If we release a RC3 now that could allow the Hibernate ORM team to be one of the initial Persistence 3.1 compatible releases.

[lukasj]

If we release a RC3 now that could allow the Hibernate ORM team to be one of the initial Persistence 3.1 compatible releases.

Isn't the requirement that the certification MUST be done against the final version of the API and final (proposed) version of the TCK (which is being promoted as I'm typing this)?

[scottmarlow]

If we release a RC3 now that could allow the Hibernate ORM team to be one of the initial Persistence 3.1 compatible releases.

Isn't the requirement that the certification MUST be done against the final version of the API and final (proposed) version of the TCK (which is being promoted as I'm typing this)?

Generally, that is best but exceptions have been made for other specifications. The TCK should also be run again after TCK + SPEC API are released as final for all initial compatible implementations (I believe we have done that with the Platform TCKs).

I believe that if Hibernate ORM 6.0 submitted a Persistence 3.1 compatibility certification request (CCR) before 3.1 is promoted to final, Hibernate ORM 6.0 would need to update their CCR request after Persistence 3.1 is final (or submit a new CCR).

[gavinking]

Can you sketch out how the type safety could look like?

Well, I would either

• overload extract, and have DateField and TimeField instead of TemporalField, or
• add a second type parameter to both TemporalField and extract().

I have not yet checked to see which way works best.

[gavinking]

(And yes, I agree that the value of the leverage of doing this is pretty marginal, which is precisely why I didn't do it in the initial proposal.)

[beikov]

Is there anything missing in this PR that prevents it from being merged? I would personally like to see this merged and a RC3 or final release soon be done with 3.1.

[gavinking]

I mean, if everyone is happy with it, and wants it, then I think it's fine.

[lukasj]

• deadline was Feb 28
• ballot is in progress: https://www.eclipse.org/lists/jakarta.ee-spec/msg02317.html with expected end on April 6

so this needs to wait for the next update and can be merged once release branch gets merged (which is going to happen after getting all approvals for the release)

[gavinking]

Folks, I've finally gotten round to making a decision on this, after chatting with @lukasj, and I've updated the PR to add a more typesafe version of the API.

[beikov]

Shouldn't we also add fields for other supported temporal types, like OffsetDateTime, ZonedDateTime and Instant?

[gavinking]

OffsetDateTime

Ideally yes, that would be nice, except that then we would be opening the whole incredible can of worms of what is the actual semantics of OffsetDateTime in JPA.

I mean, it would be pretty weird to have a OffsetDateTimeField which didn't define fields for OFFSET_HOUR and OFFSET_MINUTE, but we know that on, say, Postgres and MySQL, extracting offsets is impossible, since they're not persistent.

So I guess I would much rather not start pulling on that thread.

ZonedDateTime

JPA doesn't support ZonedDateTime.

Instant

Well, interpreted as a Java type, Instant doesn't have fields, so that's a little unnatural.

If you think this is important, then the thing I guess I would prefer to do is specify that you can cast from Instant to LocalDateTime using the cast() function.

[gavinking]

If you think this is important, then the thing I guess I would prefer to do is specify that you can cast from Instant to LocalDateTime using the cast() function.

And from OffsetDateTime to LocalDateTime too, I suppose.

[beikov]

I mean, it would be pretty weird to have a OffsetDateTimeField which didn't define fields for OFFSET_HOUR and OFFSET_MINUTE, but we know that on, say, Postgres and MySQL, extracting offsets is impossible, since they're not persistent.

True, but then we'd just return zero on these platforms since we will normalize to UTC. Of course it is platform dependent, but that IMO shouldn't stop us from providing these useful fields for a commonly used data type.

Well, interpreted as a Java type, Instant doesn't have fields, so that's a little unnatural.

It at least has an EPOCH, but I guess it's fine if we state that casting to a Long must be supported.

If you think this is important, then the thing I guess I would prefer to do is specify that you can cast from Instant to LocalDateTime using the cast() function.

SQL has this syntax for re-interpreting a timestamp into a timestamp with a specific time zone, which is what we would need here for transforming it into a OffsetDateTime: e.myInstant at time zone 'Europe/Vienna'.

[gavinking]

Of course it is platform dependent, but that IMO shouldn't stop us from providing these useful fields for a commonly used data type.

Right, but the question is: if something is so unportable and under-defined in terms of semantics, does it belong in the JPA spec? My answer to this question is usually "no". The spec should not (usually) concern itself with stuff that can't be nailed down and made nice and portable. And when in doubt, we can always add things like this later.

Anyway, with the new design, it's easy for vendors to add their own OffsetDateTimeField implementing TemporalField. It just won't be portable between JPA implementations.

But I'm not hung up on this point. I'm more like a "0" vote on this one. Let's see what @lukasj thinks. If he wants it in the spec, I'll go ahead and add it.

It at least has an EPOCH, but I guess it's fine if we state that casting to a Long must be supported.

Yeah, I think cast(instant as Long) is at-least-if-not-more intuitive than extract(epoch from instant). In principle that would be fine.

SQL has this syntax for re-interpreting a timestamp into a timestamp with a specific time zone, which is what we would need here for transforming it into a OffsetDateTime.

We don't need to cast to OffsetDateTime, all we would need would be to cast from OffsetDateTime to LocalDateTime, and I think cast(odt as LocalDateTime) works perfectly acceptably for that. So you would write:

extract(year from cast(out as LocalDateTime))

which looks fine to me, and can be implemented portably, I believe.

[gavinking]

Excuse me.

extract(year from cast(out as LocalDateTime))

To clarify: this is not necessary in JPQL, since all we say is:

It is illegal to pass a datetime argument which does not have the given field type to EXTRACT

The cast is only necessary in the criteria API.