Get rid of legacy temporal extract functions

[beikov]

I just realized that in dialects we still register the legacy temporal extract functions:

• year
• month
• day
• hour
• minute
• second
• week
• quarter

although in the parser we interpret uses like second(..) as extract(second from ..), so the functions that dialects register through:

• org.hibernate.dialect.function.CommonFunctionFactory#weekQuarter
• org.hibernate.dialect.function.CommonFunctionFactory#hourMinuteSecond
• org.hibernate.dialect.function.CommonFunctionFactory#yearMonthDay

are never called. We should either stop interpreting second(..) as extract(second from ..) or remove the unnecessary function registrations.

Note that functions in databases like e.g. second(..) might have a different return type than extract(second from ..) which can lead to some surprises when users want to use the native function.

[sebersole]

I'm guessing that you have no concrete example of this, right?

However, I do agree that it is reasonable to use the database's specific function if it defines one (second in your example) rather than the standard extract function.

As for specifics, it is SemanticQueryBuilder#visitDatetimeField you are talking about?

[gavinking]

extract(second) returns a float.

Bugger I fucked this up in the JPA spec god damn it all to hell:

https://github.com/eclipse-ee4j/jpa-api/pull/314/files

[gavinking]

I have proposed a fix jakartaee/persistence#354, let's hope it gets accepted.

Now I'm going to go off and feel bad about myself for the rest of the day.

[gavinking]

God damn it it's not OK to make mistakes like this.

[sebersole]

@gavinking You've gone back and forth enough here that I'm not sure whether to say I agree or disagree ;)

From my point of view, it is surprising if -

1. My database supports a seconds function
2. I use the function seconds in HQL
3. Hibernate decides to use extract instead.

Yes... I get the argument about "consistent return type" across databases. If that is what they really care about however, they'd just use extract directly. Or we could do this "remapping" better and always coerce the values correctly at the SQL level. E.g. here we might cast to INTEGER.

Remaining consistent with documentation that was literally just written is also not super convincing to me.

[gavinking]

The issue is of portability and consistent behavior. Databases are all over the place on this, and so we have to impose our own order.

Now, if people think that the second() function should really return integer seconds and not fractional seconds then fine, OK, now we're having a different discussion about what the semantics should be. If you guys think that, then fine, document that change and implement it. I'm not convinced it would be an improvement but I really don't care.

Note that doing this would take some actual work since not all DBs have a second() function IIRC (though most do). UPDATE: "most" is an overstatement.

And it would be a functional change that you would be making because you think it's somehow better for some reason. (Nothing in the preceding discussion was proposing a change to semantics rather than to the implementation.)

What I care about is not breaking the consistent semantics across databases.

[sebersole]

It's an unfortunate, albeit understandable, choice of function names when more than one database defines explicit functions with those names. So then what are your words of advice for existing applications that want seconds to still mean seconds?

The issue is of portability and consistent behavior. Databases are all over the place on this, and so we have to impose our own order.

That is simply using different words for exactly what I said. "Yes" is perfectly acceptable and more succinct ;)

[gavinking]

It's an unfortunate, albeit understandable, choice of function names when more than one database defines explicit functions with those names.

So I believe I'm now better-able to reconstruct my reasoning on this. So I know for sure that I started out with all these seven (year, month, day, hour, minutes, seconds, microseconds) as separate functions, defined independently of extract(). But, of course, on some databases—including PostgreSQL, Oracle, SQL Server, and Sybase, among others—they're not defined natively, and so they would need to be delegated right back to extract() on those databases.

And so I guess I just thought: well, this is a waste of time maintaining two abstractions—for functions that just do the exact same thing—and it's just easier to say they're synonyms. Less code, and less for the user to need to understand.

But sure, I admit I didn't focus very much on the possibility of breaking people who're relying on second() meaning integer seconds. And that's partly I guess because I have a lot of trouble imagining why they would be ....

So then what are your words of advice for existing applications that want second to still mean seconds?

Well, see that's where honestly I'm just not sure what to say. I mean, as it stands right now floor(second(x)) is pretty close to the same thing I guess.

But the issue is this: as I said, I really have trouble imagining what anyone uses second() or extract(second ...) for!

0. I have a pretty clear idea of what people use year, month, quarter, week_of_month, and day_of_week for. They use them for calendaring applications. Similarly, I can understand how hour and minute are used in that context. But seconds just aren't used in calendaring.
1. You see, times in principle come equipped with seconds, but in practice they never have them. There are no events which occur at 12:34:23 every Wednesday, for example! Yeah sure, sure, you might be able to construct some bizarre hypothetical, but let's be realistic here: in practice, times don't have seconds, not unless they're part of a timestamp.
2. timestamps do have seconds and fractional seconds—indeed, you probably want as many fractional seconds as possible in a timestamp—but with timestamps you don't usually use extract()! You usually compare two timestamps with > or you subtract them, something like x-y by second or whatever. Extracting the fields of a timestamp doesn't seem right. Extracting fields is really meant for calendaring.
3. Now, sure, fine, you do sometimes format Timestamps for display purposes, but using extract() or second() for that seems like just about the worst way to do it. We have other much nicer ways to format timestamps that are easier to use, and aware of i18n, etc.

So I just don't know how to reason about a use case that I don't really think exists.

So, finally: alright, if you're worried about the breakage to the second() function for existing programs, then fine. That's a perfectly legitimate thing to be worried about. We can separate these things. I'm neither for it nor against it.

And you're lucky in that I actually have most of the infrastructure already there in case we need to do that. (In particular, the Dialects which do support these functions natively already register them. And the rest I guess I can just easily enough delegate to extract().

So if that's what you want me to do, I can implement that pretty quickly. At the time I worked on all this, that felt like unnecessary complexity, but whatever, it's not something I have a strong opinion about.

So just tell me if that's what you want.

[sebersole]

To resolve this, I believe we should just use the form which gives consistency across all databases (aka the generalized second function as opposed to the Dialect-specific ones) and get rid of the Dialect registrations.

Unless I am mistaken, I think that is what y'all believe as well.

[beikov]

Yes, it just feels unnecessary to have the function registrations in code when these are never used.

[gavinking]

I think they're useful documentation of what functions the database supports.