allowing HQL formulas in @Formula, @ColumnDefault, @Check, etc

[gavinking]

Something we've discussed before, but that isn't captured here, is the ability to write HQL expressions in annotations, thus gaining portability across database platforms.

The trickiest part of this is deciding how to identify that the expression is HQL rather than raw SQL.

One solution would be to have some sort of "quoting" syntax, something like

@Formula("{concat(x,y)}")` 

or @Formula("$concat(x,y)") or whatever.

An alternative would be to have a new member of @Column, so

@Formula("concat(x,y)", dialect=HQL.class)

or whatever. The advantage I see to this approach is that we could also let you write things like

@Formula("x||y", dialect=PostgreSQLDialect.class) 
@Formula("concat(x,y)", dialect=MySQLDialect.class)

that is, write the same expression in multiple dialects of SQL.

[gavinking]

I realized that, because of the persister lifecycle, supporting HQL is quite a lot harder than it looks.

Instead I propose a semi-generic way to specify dialect-specific native SQL.

See #4537.

[sebersole]

Well, let me walk that back. In fact, it's got to be possible because we do it for JPAs @OrderBy annotation

There we simply stash the OrderBy string and process it later (akin to what I said about post-process). There it is easier though as it does not affect the overall structure of the model.

Though thinking more... I guess this would not affect the structure either. Given it applies to a Column, and we really only care about the logical name at that point, we could stash these overrides on the AnnotatedColumn for processing later

[gavinking]

Yeah, I mean, some of the things I care most about here really only affect DDL generation.

[gavinking]

So let's just say we can implement this. (HQL in annotations I mean.)

Back to the original question posed above: how would we indicate the language?

We don't want to need two annotations like with JPA @OrderBy vs Hibernate @OrderBy.

[beikov]

How about a prefix in the string e.g. @Formula("hql: concat(x, y)")? I can work on this for e.g. 6.1 if we agree on the syntax.

[gavinking]

@beikov I'm fine with that, at least for a prototype, and then we could think/argue about the precise syntax.

(FTR: I don't love it, but I sure as hell don't have a better suggestion.)

[sebersole]

Overall I am not a fan of these uber-annotations (one annotation to rule them all if you will). According to this PR, all of these are perfectly legal:

@Entity
@ForDialect( orderBy=@OrderBy( ... ), .... )
@ForDialect( columnDefault=@ColumnDefault( ... ), .... )
@ForDialect( generatedColumn=@GeneratedColumn( ... ), .... )
@ForDialect( formula=@Formula( ... ), .... )
...
class TheEntity {
}

For whatever reason, y'all see this differently than I do whenever we discuss annotation design, but to me it would make much more sense to break these out. At the very least into "valid usage targets".

[gavinking]

Sure, I don't love it either. But I don't want to add a growing set of @OrderByForDialect, @FormulaForDialect, @CheckForDialect.... annotations to hibernate.annotations.

Now, well, perhaps we could make it less-bad by making them inner annotations, something like:

@Check(constraint="...")
@Check.ForDialect(dialect=OracleDialect.class, check=@Check(constraint="..."))

Now, that would work, but it cuts against the idea that I wanted to reduce the impact on the existing annotations.

[sebersole]

I understand the concern about the explosion of annotations.

However, the lack of direction for the user as defined (my example) is simply not acceptable.

[sebersole]

Exactly, like in the example I gave.

[sebersole]

Personally I'd do the separate annotations, but I get that I am consistently in the minority on this :)

But I just can't accept this as defined when it allows the user to map the nonsense I did above. Annotations should help guide them, ya know?

[sebersole]

I really do get the "explosion" aspect.

The inner classes are not a bad idea. Like them more the more I think about them.

[gavinking]

OK, I understand, let me think some more then.

[gavinking]

@sebersole what about:

@Check(constraint="...")
@ForDialect.OverrideCheck(dialect=OracleDialect.class, check=@Check(constraint="..."))

Is that better or worse than:

@Check(constraint="...")
@Check.ForDialect(dialect=OracleDialect.class, check=@Check(constraint="..."))

It's not clear to me.

[sebersole]

You mean in terms of name and placement? One of them you added overriding for (@DiscriminatorColumn) is actually a JPA annotation so we'd not be able to add a nested annotation there. So for consistency, I guess I'd vote @ForDialect.

Not a huge fan of the name ForDialect. How about DialectOverride instead.

@Check(constraint="...")
@DialectOverride.Check(dialect=OracleDialect.class, check=@Check(constraint="..."))

And maybe version (major, minor, micro) makes some sense here?

[gavinking]

How about DialectOverride instead.

That's fine, it's what I called it in the first approach I took.

[gavinking]

You mean in terms of name and placement? One of them you added overriding for (@DiscriminatorColumn) is actually a JPA annotation so we'd not be able to add a nested annotation there.

Yeah, sure, that's indeed the advantage of that approach. So let's go with that.

[gavinking]

@Check(constraint="...")
@DialectOverride.Check(dialect=OracleDialect.class, majorVersion=11, check=@Check(constraint="..."))

LGTM, i will implement that.

[gavinking]

OK, done: #4607

[gavinking]

So now this looks like:

@Check(constraints = "code > 0")
@DialectOverride.Check(dialect = H2Dialect.class,
		sameOrAfter = @DialectOverride.Version(major = 1),
		override = @Check(constraints = "code >= 1"))

Are there any more objections/feedback on this, or is it cool if I go ahead and start working on tests?

[sebersole]

Oh, I really like the version selection.

It looks good to me

[gavinking]

Oh, I really like the version selection.

Do you think it might be better to split that annotation out of there so it can be reused by other annotations in the future?

[gavinking]

The @Version annotation I mean.

[sebersole]

I'd say leave it as it is until we find such a case. We can always add a new top-level @DialectVersion later if the need arises and support both on @DialectOverride

[gavinking]

OK, fine. It's true that I don't have a motivating use for it.

[gavinking]

Anyway I'll mark this shit @Incubating.