HHH-19506 Improve FirebirdDialect

[mrotteveel]

This PR improves the Firebird dialect:

• Use Firebird 3.0 as dialect default (was Firebird 2.5)
• Fixes issue with TIME generated as time(p)
• Adds insert function support
• Adds repeat function support
• Adds hypothetical ordered set aggregates support (Firebird 3+)
• Uses SHA-256 for sha function (instead of SHA1) (Firebird 4+)
• Adds hex function support (Firebird 4+)
• Adds generate_series function support (emulated)
• Raises IN limit for Firebird 5+ to 65535
• Add skip locked support (Firebird 5+)
• Improved constraint violation detection by specifying the type of constraint violated
• Fixed "on conflict" INSERT to use MERGE
• Fixed UPDATE with join to use MERGE
• Fixed AST getForShare to return with lock instead of for update
• Fixed AST visitValuesTableReference to emulate values table
• Improved casting of parameters in cases where Firebird can't (always) infer the type of a parameter
• Fixed issue with generated DML using a table alias where the table did not have that alias rendered

Includes changes to either ignore tests for Firebird, or get tests running on Firebird. Also improved robustness of some tests.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

---

https://hibernate.atlassian.net/browse/HHH-19506

[gavinking]

Wow, Firebird has a lot of features these days!

[mrotteveel]

Looking at the build failure now.

[hibernate-github-bot]

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

[mrotteveel]

@gavinking I think I fixed the build failures, except the failures in the GraalVM 21 - oracle_atps, as I don't see how that could be related to my changes.

[gavinking]

No, the Oracle ATPS failures are surely unrelated to your change.

[gavinking]

LGTM

[mrotteveel]

Can this be merged, or is more needed (I see the Jenkins pipeline is waiting for some input?)

[mbellade]

@mrotteveel thanks for the contribution, I trust you've tested your changes with Firebird and ensure they work as expected.

Before merging I have one note: usually, we don't make changes to hibernate-core's test suite to account for community dialects. If we were to do that for all of them, the code would become riddled with custom logic / test skips for dialects that the Hibernate team doesn't know much about.

I see you've changed a lot of the core tests. While e.g. adding / changing skips from specific dialects to a DialectFeatureCheck is totally fine, I would ask you avoid referencing FirebirdDialect directly, and also try to avoid changing the test logic to account for specifics of a community dialect.

[gavinking]

Before merging I have one note: usually, we don't make changes to hibernate-core's test suite to account for community dialects.

Not disagreeing with you Marco, but I just want to note that I think we should so some flexibility in this specific case because @mrotteveel really does put in a lot of work to keep the tests running on Firebird, which is not the case for most (all?) of the other community dialects.

[mrotteveel]

@mbellade I ran the tests, which is why I made those changes to account for differences. Most of the changes I made, I did in a way that makes it not dialect-specific (though in one or two cases I did assume SQL standard syntax for quoted identifiers). Some of the other changes I made, I made because some of the tests were brittle due to unclosed resources on failures (Firebird is unfortunately pretty sensitive when it comes to performing DDL if there is still a prepared statement using a table). I also made changes like this when making previous contributions to the Firebird dialect.

The tests in hibernate-core already have references to the following community dialects: Firebird, Derby, Altibase, Informix, TiDB.

I think that if you want maintenance of community dialects, you will need to have those things in your tests; otherwise testing is quite hard to do, because either I'd need to maintain my own fork or patches to reapply each and every time I want to work on the dialect, or I need to re-investigate each and every test failure when I do some work.

You (as the Hibernate team) don't need to maintain things requiring or ignoring a community dialect: that will be the responsibility of the contributors helping with maintenance of those dialects.

In any case, do you want me to remove the changes to the hibernate-core tests directly referencing the dialect?

[mbellade]

@mbellade I ran the tests, which is why I made those changes to account for differences. Most of the changes I made, I did in a way that makes it not dialect-specific (though in one or two cases I did assume SQL standard syntax for quoted identifiers). Some of the other changes I made, I made because some of the tests were brittle due to unclosed resources on failures (Firebird is unfortunately pretty sensitive when it comes to performing DDL if there is still a prepared statement using a table). I also made changes like this when making previous contributions to the Firebird dialect.

If you are generally improving tests, without adding logic for a specific community-dialect, I would ask you to create create a Jira for it (which briefly explains the reason you're proposing such changes) and apply the changes in in a separate PR or, at least, in specific commits unrelated to Firebird.

The tests in hibernate-core already have references to the following community dialects: Firebird, Derby, Altibase, Informix, TiDB.

That's probably an artifact of when (some of) those dialects were not community.

You (as the Hibernate team) don't need to maintain things requiring or ignoring a dialect: that will be the responsibility of the contributors helping with maintenance of those dialects.

But we do need to maintain the test suite, which might involve changing one of the tests that reference community dialects or which logic was changed to account for that and cause additional complexity on our side.

In any case, do you want me to remove the changes to the hibernate-core tests directly referencing the dialect?

It is my opinion we should try to avoid that, yes, but if you feel that shouldn't be the case I would leave it up for discussion with the rest of the Hibernate team.

Edit: @gavinking just giving my opinion, I don't see anything majorly concerning with these changes but if more and more community dialects were to do this test suite maintenance could become a problem.

[gavinking]

@mbellade Yes I understand the concern, just saying that I think Firebird is in a bit of a category by itself here.

[mrotteveel]

@mbellade I ran the tests, which is why I made those changes to account for differences. Most of the changes I made, I did in a way that makes it not dialect-specific (though in one or two cases I did assume SQL standard syntax for quoted identifiers). Some of the other changes I made, I made because some of the tests were brittle due to unclosed resources on failures (Firebird is unfortunately pretty sensitive when it comes to performing DDL if there is still a prepared statement using a table). I also made changes like this when making previous contributions to the Firebird dialect.

If you are generally improving tests, without adding logic for a specific community-dialect, I would ask you to create create a Jira for it (which briefly explains the reason you're proposing such changes) and apply the changes in in a separate PR or, at least, in specific commits unrelated to Firebird.

Well, in this case it's pretty much both: all the changes I made was to address test failures (some intermittent, some cascading to other tests) when running the tests against Firebird. I did try to do it in a non-dialect specific way.

The tests in hibernate-core already have references to the following community dialects: Firebird, Derby, Altibase, Informix, TiDB.

That's probably an artifact of when (some of) those dialects were not community.

Could be, though Altibase is a recent addition.

You (as the Hibernate team) don't need to maintain things requiring or ignoring a dialect: that will be the responsibility of the contributors helping with maintenance of those dialects.

But we do need to maintain the test suite, which might involve changing one of the tests that reference community dialects or which logic was changed to account for that and cause additional complexity on our side.

I understand that, but Require/Ignore dialect annotations are not involved with that, so referencing dialects there should - in my opinion - not be a problem for your maintenance

The only real logic changes I made were in:

• BaseInsertOrderingTest/InsertOrderingSelfReferenceTest: to allow checks against different, but SQL standard conformant, expected syntax (parameter is a SQL:2003 reserved word, and quoting it somehow also causes parent_id to become quoted).
  That made it hard to solve differently; I initially tried to get an identifier helper to quote, but that wouldn't quote parent_id as it doesn't actually need quoting, but some logic deep in Hibernate will quote it if the table name is also quoted)
• SubQueryTests: added a trim because in Firebird the column becomes a CHAR(5) leading to a trailing space; I'm surprised not more dialects have this issue
• LeftRightReplaceTest: split the test so it could be ignored separately
• SimpleQuerySpecificationTests: to allow checks against different, but SQL standard conformant, expected syntax (position is a SQL:2003 reserved word)
• FunctionTests: addressed issue with one failure cascading to subsequent failures due to deletion of test data on failure, and split off a test so it could be ignored separately
• RecordIdClassTest/MergeRecordEmbeddedIdTest: made change to prevent self-referencing foreign key to prevent deletion; might not be needed for other dialects, but doesn't harm them
• OverrideStandardJavaTypeTest: language is a SQL:2003 reserved word, though I guess I could also have addressed it by giving it a different, non-reserved, name

I also modified some base tests to put the session cleanup in a finally to ensure it runs, or to catch exceptions when closing multiple resources (sessions/factories) to prevent a failure to close one from closing others.

In any case, do you want me to remove the changes to the hibernate-core tests directly referencing the dialect?

It is my opinion we should try to avoid that, yes, but if you feel that shouldn't be the case I would leave it up for discussion with the rest of the Hibernate team.

Edit: @gavinking just giving my opinion, I don't see anything majorly concerning with these changes but if more and more community dialects were to do this test suite maintenance could become a problem.

I think that this could be argued for the seven things I listed above. I did try and keep those changes not dialect specific, but I agree that (at least some of them) increase complexity.

Are those specifically the things (or things like that) you have a problem with, or do you also have a problem with adding skips for a community dialect? Because I don't see how that adds to maintenance burden for you (if you're not sure, leave it in place, or just delete it, and the next time a community dialect maintainer works on the dialect, they'll recheck it themselves)

[mbellade]

@mrotteveel as I said, I'm not worried about improving test stability - that's great, it would be nice to be able to track it to a separate pr / commit. Usually, we discourage skips for community dialects, but I agree that's not the main issue. What concerns me most is changes to a tests logic (i.e. assertions, order of operations, sql syntax) that need to account for community dialect specific feature support.

It's not like this PR fundamentally changes the core test suite, so I'm not strongly against merging it, I'm just trying to remind everyone of our usual community dialect practices.

[mrotteveel]

OK, understood. Thanks!

[gavinking]

For context @mrotteveel, we have in the past rejected a PR which wanted to add many skips for a brand-new community dialect.

[mrotteveel]

I also removed one of my skips for Firebird, because I actually committed a fix to Firebird yesterday which will address the skip reason in supported Firebird versions.