Skip to content

Update implementations of PagingQueryProvider #4713

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

Closed
wants to merge 2 commits into from

Conversation

hpoettker
Copy link
Contributor

Resolves #1673 and #1253.

As reported in #1253, the current implementation of DerbyPagingQueryProvider doesn't really work, which can be confirmed by running the added DerbyPagingQueryProviderIntegrationTests against the current implementation.

This PR replaces the implementation of DerbyPagingQueryProvider effectively with that of Db2PagingQueryProvider as suggested by #1253 and is tested against Derby in the added DerbyPagingQueryProviderIntegrationTests. The build uses Derby version 10.16.1.1, which is the first and only release with a baseline of Java 17 as the next Derby release already requires Java 21.

I took the liberty of deprecating SqlWindowingPagingQueryProvider for removal. The only PagingQueryProvider that currently uses its implementation is DerbyPagingQueryProvider. The other 3 subclasses (and with this PR also DerbyPagingQueryProvider) don't actually depend on its functionality as they override any relevant methods.

@hpoettker hpoettker force-pushed the derby-maintenance branch 2 times, most recently from 5216dd9 to 3e490ff Compare November 24, 2024 20:05
@hpoettker
Copy link
Contributor Author

I've pushed a second commit that includes not only an integration test for Derby, but also for Db2, HSQL, MariaDB, MySQL, Oracle, PostgreSQL, Sql Server, and SQLite.

The setup is mostly from the job repository integration tests. For Oracle, I've switched the tests to the publicly available image from here: https://github.com/gvenzl/oci-oracle-xe/

This should allow to also close #1430, as the PR proposes to remove SqlWindowingPagingQueryProvider anyway, and the tests show that OraclePagingQueryProvider should work as expected.

The two tests in AbstractPagingQueryProviderIntegrationTests are currently quite minimal. In the future, it would be interesting to also investigate queries with joins on columns with the same name. There are some open bug reports about this situation.

@fmbenhassine
Copy link
Contributor

Hi @hpoettker ,

This is a huge amount of work! Thank you for all these updates 👍

For derby's version, we will have to stay on that version as I am not sure Spring Batch 6 (based on Spring framework 7) will have Java 21 as a baseline. Our first plan is to stay on Java 17 for the next major versions.

For oracle, I was aware of that base image, but unfortunately we cannot use it in our CI/CD pipeline as it is not an official image (even though it is from an Oracle employee, this is an internal policy). So please revert that change.

Lastly, the deprecation of the SQL windowing provider comes at the right time, and I agree on the change.

Other than that, I see nothing blocking for merging this into a patch release.

Please update the PR and it will be good to merge for the upcoming 5.2.1 (no backports though).

Best regards,
Mahmoud

Replaces the implementation of `DerbyPagingQueryProvider` with that corresponding to DB2 and adds an integration test that failed with the previous implementation.

Deprecates `SqlWindowingPagingQueryProvider` for removal, which was effectively only used by `DerbyPagingQueryProvider`.
Adds testcontainers based tests for DB2, MySQL, MariaDB, Postgres, Sql Server and Oracle Database, as well as standard tests for HSQL and SQLite.
@hpoettker
Copy link
Contributor Author

done

@fmbenhassine
Copy link
Contributor

Perfect 👍 Rebased and merged. Thank you for this awesome contribution!!

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

Successfully merging this pull request may close these issues.

Update DerbyPagingQueryProvider for Derby 10.7 and above
2 participants