Skip to content

HHH-19694 Enhanced support for older Informix #10706

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

VladoKuruc
Copy link
Contributor

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


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.


@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Aug 7, 2025

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

@beikov
Copy link
Member

beikov commented Aug 7, 2025

Can you please prefix all commits with the Jira key HHH-19694 and maybe also squash the commits?

Copy link
Member

@gavinking gavinking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the changes to the Dialect itself are OK but I don't see why there should be any changes to tests here.

The test suite was already all passing on the current release of Informix, and we don't need to be able to test on obsolete versions of Informix. (We can't, actually, since we don't have access to any.)

So unless there's some other good reason for these test changes (other than to make the tests work on obsolete versions), please back out those changes. Thanks.

@gavinking
Copy link
Member

pfffffff, oh gawd I just noticed we're already back up to 26 failures (from zero) on main, including some stuff that really should not have been able to break, after relatively few changes. I guess we should add an Informix CI job.

@VladoKuruc
Copy link
Contributor Author

I guess the changes to the Dialect itself are OK but I don't see why there should be any changes to tests here.

The test suite was already all passing on the current release of Informix, and we don't need to be able to test on obsolete versions of Informix. (We can't, actually, since we don't have access to any.)

So unless there's some other good reason for these test changes (other than to make the tests work on obsolete versions), please back out those changes. Thanks.

The tests with the annotation @RequiresDialectFeature(DialectChecks.SupportsDmlTargetColumnQualifier.class) relate to the bug at https://hibernate.atlassian.net/browse/HHH-18473. This issue was fixed for a newer version of Informix by switching to TABLE_ALIAS. I thought it would be beneficial to mark these tests in case a fix for the NONE mode is ever implemented in the framework itself.

I'm happy to revert the other set of tests where I changed count(1) to count(*), as it seems only the old Informix version has a problem with it.

@gavinking
Copy link
Member

I thought it would be beneficial to mark these tests in case a fix for the NONE mode is ever implemented in the framework itself.

I don't understand what you're saying. DynamicFIlterTest passes for me. Why would we skip it?

@VladoKuruc
Copy link
Contributor Author

I thought it would be beneficial to mark these tests in case a fix for the NONE mode is ever implemented in the framework itself.

I don't understand what you're saying. DynamicFIlterTest passes for me. Why would we skip it?

Only skip for old Informix:

public static class SupportsDmlTargetColumnQualifier implements DialectCheck {
    @Override
    public boolean isMatch(Dialect dialect) {
       return dialect.getDmlTargetColumnQualifierSupport() != DmlTargetColumnQualifierSupport.NONE;
    }
}
...
@Override
public DmlTargetColumnQualifierSupport getDmlTargetColumnQualifierSupport() {
    return getVersion().isSameOrAfter( 12,10 ) ? DmlTargetColumnQualifierSupport.TABLE_ALIAS : DmlTargetColumnQualifierSupport.NONE;
}

@gavinking
Copy link
Member

Only skip for old Informix:

Ok, so what I'm saying is we don't want anything in the test suite which is specific to obsolete versions of Informix.

@beikov
Copy link
Member

beikov commented Aug 12, 2025

Only skip for old Informix:

Ok, so what I'm saying is we don't want anything in the test suite which is specific to obsolete versions of Informix.

I'm not sure what it is exactly that you're against here, but having a @RequiresDialectFeature that ensures the Dialect has a DmlTargetColumnQualifierSupport other than NONE seems ok to me. Before, he used @SkipDialect for the these tests to skip on the old Informix version, which was way worse. I understand that you don't want to see or deal with such skips for vendor unsupported versions, but it's not like we are "supporting" Informix anyway. So if @VladoKuruc wants to do the work and ensure the testsuite also works with older database versions in an IMO non-invasive manner, I'd not want to get in the way of merging such small changes if it helps him.
The alternative might be that he maintains a list of tests to skip somewhere external to the tests (through the -PexcludeTests=**.MutationQueriesFilterTest CLI parameter), or runs a patched version of the testsuite against the older version of Informix locally, which might also work for him.

Wdyt @sebersole?

@gavinking
Copy link
Member

@beikov the PR introduces five instances of:

@SkipForDialect( dialectClass = InformixDialect.class, majorVersion = 11, minorVersion = 50, reason = "Informix does not support ....")

Informix 11 is a thoroughly-obsolete EOL version of the database, which we have no way to even run, let alone test against.

I don't see how that's "non-invasive".

@VladoKuruc
Copy link
Contributor Author

There are no @SkipForDialect annotations anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants