Skip to content

Add MySQL as a supported database for the relational JDBC persistence backend#4281

Open
Y-Wakuta wants to merge 1 commit into
apache:mainfrom
Y-Wakuta:feat/mysql-support
Open

Add MySQL as a supported database for the relational JDBC persistence backend#4281
Y-Wakuta wants to merge 1 commit into
apache:mainfrom
Y-Wakuta:feat/mysql-support

Conversation

@Y-Wakuta

@Y-Wakuta Y-Wakuta commented Apr 23, 2026

Copy link
Copy Markdown

This PR continues the work @Jayden-Chiu started in #2704 and owes its overall shape to that earlier effort — the DatabaseType enum extension, the MySQL toMap branching pattern, the testcontainer-based MySQL lifecycle management, and the per-subclass integration-test layout are all derived from @Jayden-Chiu's original proposal. Where this PR differs, it is to incorporate the review feedback (from @adutra, @dimas-b, @eric-maynard, @jbonofre, @flyrain) that landed on #2704 and #2491 after that PR went stale. Concretely:

Changes

Persistence layer

  • Added MYSQL to the DatabaseType enum, with product-name detection in inferFromConnection and display-name support for "mysql". (Follows the enum-addition pattern from Add support for MySQL Metastore #2704.)
  • Added MySQL SQLSTATE handling to DatasourceOperations:
    • 23000 (integrity constraint violation) combined with MySQL vendor error 1062 (duplicate entry) to match PostgreSQL's 23505 precision. MySQL does not have a dedicated SQLSTATE for unique-key violation.
    • 42S02 (table not found) — same code as H2.
  • Added mysql/schema-v4.sql (the structure follows the tables Jayden-Chiu drafted in Add support for MySQL Metastore #2704, upgraded to the current v4 layout). Notable MySQL-specific adjustments:
    • JSONBJSON; ON CONFLICTON DUPLICATE KEY UPDATE; JSON columns use DEFAULT ('{}') to match PostgreSQL's default.
    • USE POLARIS_SCHEMA; in place of CREATE SCHEMA ... SET search_path (in MySQL, a schema is a database; the database is provisioned via the JDBC URL / testcontainer).
    • Indexes declared inside CREATE TABLE because MySQL lacks CREATE INDEX IF NOT EXISTS. The bootstrap re-runs this script per realm, and idempotency is covered by CREATE TABLE IF NOT EXISTS.
    • entities.name sized to VARCHAR(256) to match Polaris' MAX_IDENTIFIER_LENGTH while staying within the InnoDB 3072-byte key length for the (realm_id, ..., name) unique key.
    • location_without_scheme indexed with a 400-char prefix for the same key-length reason.
  • JSON-column handling for MySQL. MySQL's JDBC driver (Connector/J) maps JSON columns to java.lang.String and has no Types.JSON, so a bound VARCHAR compared against a JSON column in a WHERE clause is always type-different / never-equal under MySQL's JSON comparison rules — which was the root cause of MysqlPolicyServiceIT failing on the policy-mapping detach path. (Not caught in Add support for MySQL Metastore #2704; surfaced here because the integration test suite exercises detach/drop end-to-end.) This PR addresses it with a static schema lookup rather than any value-level marker:
    • A new ModelRegistry records which (table, column) pairs are JSON columns; each Model* class declares a JSON_COLUMNS set.
    • QueryGenerator consults ModelRegistry.isJsonColumn(table, column) when building WHERE equality conditions and, for JSON columns, emits the placeholder returned by DatabaseType.asJsonConditionPlaceholder()CAST(? AS JSON) for MySQL and the plain ? for PostgreSQL / CockroachDB / H2.
    • On the write side, the previously copy-pasted if (POSTGRES) toJsonbPGobject(...) else raw-string branches in ModelEntity, ModelEvent, ModelPolicyMappingRecord, ModelCommitMetricsReport and ModelScanMetricsReport are consolidated into a single shared helper Converter#wrapJsonForDatabase(json, databaseType) (PostgreSQL wraps to a PGobject; MySQL/H2/CockroachDB pass the raw JSON string, which MySQL implicitly parses into the JSON column on INSERT/UPDATE).
    • ModelRegistryTest and updates to QueryGeneratorTest cover the new path.
  • JdbcMetaStoreManagerFactory.produceDatasourceOperations selects between the default (unnamed) datasource and a named MySQL datasource based on the configured database-type, failing fast with actionable guidance if mysql is configured but its named datasource is unavailable.

Runtime module

  • The standard runtime/server is the runner used for every relational backend; the MySQL JDBC driver is added to it (as runtimeOnly) only when -PincludeMysqlDriver=true is passed to Gradle, which also sets quarkus.datasource.mysql.jdbc=true at Quarkus build time so the named MySQL datasource is wired up.

  • Because the GPL driver is intentionally absent from LICENSE, the license-report / verification / publish tasks are not supported under -PincludeMysqlDriver=true and will fail by design — :polaris-server:build, :check, :publishToMavenLocal, :generateLicenseReport, :checkLicense, :licenseReportZip. Only :assemble and the Docker image build are supported with the flag (the unsupported tasks are listed in runtime/server/build.gradle.kts).

  • The default release build remains GPL-free:

    ./gradlew :polaris-server:assemble # ASF compliant (no MySQL driver)
    ./gradlew :polaris-server:assemble -PincludeMysqlDriver=true # opt-in custom downstream build

  • The Quarkus runtime distinguishes between the default (unnamed) datasource — used by PostgreSQL, CockroachDB and H2 — and the named MySQL datasource. The named MySQL datasource is declared inactive (quarkus.datasource.mysql.active=false, quarkus.datasource.mysql.jdbc=false) in runtime/defaults/.../application.properties; users opt in at runtime by setting quarkus.datasource.mysql.active=true and the connection properties on quarkus.datasource.mysql.*.

Test infrastructure

  • A new, dedicated Gradle module persistence/relational-jdbc-mysql/tests (project :polaris-relational-jdbc-mysql-tests) holds the MySQL integration tests. It is an integration-test-only module that pulls the GPL MySQL JDBC driver (io.quarkus:quarkus-jdbc-mysql), so it is not a published library and is explicitly excluded from the BOM (bom/build.gradle.kts) and registered in gradle/projects.main.properties. Keeping the driver in this module keeps it off the production-facing runtime/service classpath.
  • The module's intTest source set contains MysqlRelationalJdbcLifeCycleManagement, MysqlRelationalJdbcProfile, and the MysqlApplicationIT / MysqlManagementServiceIT / MysqlPolicyServiceIT / MysqlRestCatalogIT / MysqlViewFileIT tests. The *IT classes extend the shared Polaris integration-test base classes from org.apache.polaris.service.it.test (the :polaris-tests / integration-tests module) and reuse the existing ServerManager (the PolarisServerManager SPI impl) from runtime/service test fixtures rather than duplicating it; the SPI is registered via a META-INF/services/org.apache.polaris.service.it.ext.PolarisServerManager resource in this module.
  • MysqlRelationalJdbcLifeCycleManagement starts a MySQLContainer (image pinned via Dockerfile-mysql-version, currently mysql:8.0, Renovate-managed), provisions the POLARIS_SCHEMA database, and injects the runtime config the tests need: polaris.persistence.relational.jdbc.database-type=mysql, quarkus.datasource.mysql.active=true, and the container's JDBC URL / credentials. quarkus.datasource.mysql.jdbc=true is set for this module at Quarkus build time.
  • Two cross-backend regression tests were added to the shared integration-test classes and therefore run against all backends (PostgreSQL, CockroachDB, H2, MySQL): a case-sensitivity coexistence test in PolarisManagementServiceIntegrationTest and a non-trivial-JSON attach/detach test in PolarisPolicyServiceIntegrationTest (the latter exercises the JSON-column WHERE-clause path described above).

Documentation

  • persistence/relational-jdbc/MYSQL.md: a new doc describing the MySQL backend, the GPL-driver opt-in build, and runtime configuration.
  • runtime/server/README.md: a "Building a MySQL-capable server (custom downstream build)" note: GPL-driver disclaimer, -PincludeMysqlDriver=true build commands (including the Docker image build), and runtime configuration for the named MySQL datasource.
  • site/content/in-dev/unreleased/metastores/relational-jdbc.md: a paragraph noting that the MySQL path requires a custom downstream build from the official source release and pointing to the source-tree README; the closing paragraph clarifies that official Polaris release artifacts support PostgreSQL and H2, with MySQL available only via a custom source build.
  • CHANGELOG.md: entry framed as "source-level support", with an explicit statement that official Polaris release artifacts do not include the MySQL JDBC driver.

Impact on existing backends

None. PostgreSQL, CockroachDB and H2 users do not need to change any configuration: they continue to use the default (unnamed) datasource via quarkus.datasource.*. The only MySQL-aware code on the shared persistence path is the DatabaseType switch in QueryGenerator (asJsonConditionPlaceholder() returns CAST(? AS JSON) for MySQL and ? for every other backend) and the MySQL branches in DatasourceOperations.isUniquenessConstraintViolation / isRelationDoesNotExist, all gated on DatabaseType.MYSQL. Non-MySQL backends take the existing code paths unchanged, and the Converter#wrapJsonForDatabase refactor preserves the prior PostgreSQL PGobject / raw-string behavior.

How it was tested

  • ./gradlew :polaris-relational-jdbc-mysql-tests:intTest -PincludeMysqlDriver=true — the full MySQL integration-test suite passes against a testcontainer-managed mysql:8.0: 482 tests, 0 failures (MysqlApplicationIT 18, MysqlManagementServiceIT 52, MysqlPolicyServiceIT 47, MysqlRestCatalogIT 313, MysqlViewFileIT 52). These exercise bootstrap, the Polaris management API, policy attach/detach with non-trivial JSON parameters, and the Iceberg REST catalog/view CRUD flows end-to-end.
  • ./gradlew :polaris-runtime-service:intTest --tests "*JdbcApplicationIT" --tests "*CockroachApplicationIT" — the existing PostgreSQL and CockroachDB *ApplicationIT suites continue to pass, confirming no regression from the shared-path changes.
  • ./gradlew :polaris-server:assemble (no opt-in flag) succeeds and :polaris-server:quarkusBuild produces a GPL-free runner; the license-report tasks pass.
  • ./gradlew :polaris-server:assemble -PincludeMysqlDriver=true produces a runner with the MySQL JDBC driver bundled.
  • ./gradlew :polaris-relational-jdbc:test — unit tests (including ModelRegistryTest and the updated QueryGeneratorTest) pass.

Acknowledgments

Thanks to @Jayden-Chiu for the original work in #2704 — its DatabaseType extension, schema-file layout, and test-infrastructure patterns are the foundation of this PR. Thanks also to @adutra, @dimas-b, @eric-maynard, @jbonofre, @flyrain, @singhpk234, and @snazy for the review feedback on #2704, #2491 and this PR that shaped the design.

AI-assisted contribution

Parts of this change were drafted with AI assistance (Claude). The author (@Y-Wakuta) reviewed, tested, and takes full responsibility for the final code.

Fixes #2491
Continues #2704

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes Support MySQL as Metastore #2491
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

@github-project-automation github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Apr 23, 2026
@Y-Wakuta
Y-Wakuta force-pushed the feat/mysql-support branch 3 times, most recently from 2b48aeb to 72a72f3 Compare April 23, 2026 11:03
@flyrain
flyrain requested a review from singhpk234 April 23, 2026 16:34
@Y-Wakuta
Y-Wakuta force-pushed the feat/mysql-support branch 3 times, most recently from e20ebf9 to 0ac5949 Compare April 24, 2026 04:25
@Y-Wakuta
Y-Wakuta marked this pull request as ready for review April 24, 2026 04:26
Comment on lines +187 to +189
* Builds a column-to-value map containing exactly the primary-key columns of the
* policy_mapping_record table. Keep this in sync with the {@code PRIMARY KEY (...)} clause of
* each schema-vN.sql file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't fully understand, why there is this restriction ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks @singhpk234 — your comment prompted me to dig in, and the answer turned out to be more general than the v1 javadoc captured.

Background: MysqlPolicyServiceIT.testPolicyMapping was failing with PolicyInUseException because detachPolicy wasn't deleting the mapping row. My initial hypothesis was JSON canonicalization, and toPrimaryKeyMap was a workaround: drop the JSON column from the WHERE and match by PK only.

The actual issue: canonicalization didn't fit (the test uses Map.of(), an empty {} with nothing to canonicalize). The real cause is structural — JDBC has no Types.JSON so Connector/J maps MySQL JSON to
java.lang.String
, the bound VARCHAR doesn't compare as JSON-vs-JSON, and under MySQL's JSON comparison rules the predicate evaluates as JSON OBJECT vs JSON scalar (different types, never equal).

The fix (latest commit): mirror the existing PostgreSQL toJsonbPGobject pattern with a Converter.MysqlJsonValue marker, and have QueryGenerator emit column = CAST(? AS JSON) when it sees one. Converter#wrapJsonForDatabase centralizes the per-database wrapping; DatasourceOperations.bindParam unwraps MysqlJsonValue before setObject, gated on MySQL only so PostgreSQL / CockroachDB / H2 paths are untouched. This covers every JSON column in the schema (entities.*, events.additional_properties, *.metadata), not just parameters, and lets deleteFromPolicyMappingRecords use the shared ALL_COLUMNS path on every backend — toPrimaryKeyMap and deleteFromPolicyMappingRecordsMySql are removed.

) COMMENT = 'the version of the JDBC schema in use';

INSERT INTO version (version_key, version_value)
VALUES ('version', 4)

@flyrain flyrain Apr 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We may aim for v5 as 1.4.0 is out with v4. I also expect schema changes in 1.5.0.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Bumping to v5 for 1.5.0 sounds reasonable. If there are schema changes or discussions I should align with, please point me at them and I'll incorporate them here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There are some discussions about metrics schema in the dev mailing list, but it won't affect this PR. We can handle it once we made the metric changes.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the context. Happy to follow up on the MySQL side once the metrics schema changes are in.

@Override
public void deleteFromPolicyMappingRecords(
@Nonnull PolarisCallContext callCtx, @Nonnull PolarisPolicyMappingRecord record) {
if (datasourceOperations.getDatabaseType() == DatabaseType.MYSQL) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This branching is unfortunate. The MySQL path deletes by PK only; can't we use this path for all database types?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed — the branching is removed in the latest commit. MySQL now uses the same ALL_COLUMNS WHERE path as PostgreSQL/H2 via a new Converter.MysqlJsonValue marker (symmetric to toJsonbPGobject for PostgreSQL). Background and verification under @singhpk234's thread.

@Y-Wakuta
Y-Wakuta requested review from adutra and singhpk234 April 30, 2026 08:24
@snazy

snazy commented Apr 30, 2026

Copy link
Copy Markdown
Member

Hi @Y-Wakuta,

Thanks for pushing this forward. I have not reviewed the implementation in technical detail yet, so I am not commenting on the concrete technical approach here. I do want to be fairly firm on one release/docs point, though, so we keep the user-facing release story precise about what Polaris officially ships and what requires a custom downstream build.

Because the MySQL JDBC driver is not part of the official Polaris artifacts, I do not think we should present this in released docs/changelog as ordinary end-user support, and released documentation should not send users to raw GitHub tree / main-branch content for this path.

Concretely, when this lands, I'd like us to make sure that:

  • released docs and release notes do not link to runtime/server-mysql/README.md on GitHub or other raw repository content for this;
  • if we mention where users can find the detailed steps, released docs should direct them to the official Polaris source release, and then to runtime/server-mysql/README.md in the unpacked source tree;
  • detailed custom-build instructions stay in that source-release/developer context, not in versioned end-user docs or release notes;
  • we avoid unqualified wording such as "MySQL is a supported database" unless the official Polaris release artifacts themselves support that path;
  • the wording clearly distinguishes official Polaris release artifacts from a custom downstream build produced from the official source release.

For example, I think wording along these lines would be much safer:

Added source-level support for building a MySQL-specific runtime for the relational JDBC backend. Official Polaris artifacts do not include the MySQL JDBC driver. Users who need this path should download the official Polaris source release and build their own derivative from that source tree; see runtime/server-mysql/README.md in the unpacked source release for details.

I am not trying to turn this into a blocker for the implementation itself, but I do want us to treat the release/docs framing as part of getting this over the line, rather than as an optional follow-up.

* Maven group:artifact IDs: org.graalvm.truffle:truffle-runtime

Home page: https://github.com/oracle/graal/
License: The GNU General Public License (GPL) - https://github.com/oracle/graal/blob/master/LICENSE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's clearly a no-go here. I don't think we have to provide any LICENSE and NOTICE here as we don't publish the server-mysql distribution as part of our "official" artifacts.

To avoid any confusion about the purpose of the server-mysql, I would just remove the distribution folder containing LICENSE and NOTICE.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed, that's the cleaner positioning. Removed the distribution folder along with the now-unused distributionElements / licenseNoticeElements Gradle configurations in the latest commit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry if I wasn't clear. My proposal is to completely remove runtime/server-mysql.
We should not have additional runtime/server in Polaris.

Instead we should:

  1. As suggested by @adutra , we should have (in the regular server) pre-defined db-kind configuration (postgresql, mysql, ...) by default. It's resolved at runtime injecting the corresponding bean.
  2. We should document how users can assembly and build custom server runtime.
  3. During the Community Meeting yesterday, I proposed to work on a "tool" to facilitate custom runtime assembly for our users.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Got it — agreed on removing runtime/server-mysql. To map to your three points: (1) I'll fold the MySQL configuration into runtime/server using @adutra's approach, (2) I'll add documentation for users to assemble their own custom MySQL-capable runtime, and (3) the assembly tool sounds like a great direction. I'd assume the tool itself is beyond this PR's scope, but happy to lay any groundwork here that would help — please point out anything you'd like included.

grant_records_version INT NOT NULL COMMENT 'the version of grant records change on the entity',
location_without_scheme TEXT,
PRIMARY KEY (realm_id, id),
CONSTRAINT constraint_name UNIQUE (realm_id, catalog_id, parent_id, type_code, name),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

constraint_name is a weird name.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@adutra
Agreed that constraint_name is an unnatural name. However, this same name is currently used in the postgres, h2, and cockroachdb schemas as well. One option would be to rename it to something like uq_entities_identity, but that would introduce a divergence between the MySQL schema and
the others that isn't driven by any DB-specific behavior.

Long-term I think renaming the constraint across all DB schemas is the right move, but I'd like to avoid changing the non-MySQL schemas in this PR. Which of the following do you think is more appropriate?

  1. Use a better name (e.g. uq_entities_identity) only in the MySQL schema — improves the name, but introduces a temporary divergence from the other DBs.
  2. Keep the same constraint_name as the other DBs in this PR — consistent across all schemas, defers the rename entirely.

Either way, if we don't touch the other schemas here, I'd like to open a follow-up PR to rename the constraint across all DB schemas.

Comment thread gradle/projects.main.properties Outdated
polaris-runtime-defaults=runtime/defaults
polaris-runtime-service=runtime/service
polaris-server=runtime/server
polaris-server-mysql=runtime/server-mysql

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm a bit disappointed that we need to introduced a new server module especially for MySQL.

I suppose this is being done either because of licensing issues, or because quarkus.datasource.db-kind is a build-time property.

The licensing issues are not a problem as long as the category X dependency is gated behind a build property, and is strictly opt-in. The LICENSE and NOTICE files should not reflect these dependencies since they are not released in any of the official Polaris releases.

As for the build-time issue: it is possible to workaround that by leveraging named datasources. The same project can declare multiple datasources, and the selection of the active one happens at runtime.

Project Nessie uses this strategy, see how different datasources are configured there:

https://github.com/projectnessie/nessie/blob/b37627031e4bec547b10890d5e7ed50027481963/servers/quarkus-server/src/main/resources/application.properties#L220-L252

The runtime activation of the datasource is done with a ConfigSourceInterceptor:

https://github.com/projectnessie/nessie/blob/522010a484ddc1cc77148412f137390db9bc15cd/servers/quarkus-common/src/main/java/org/projectnessie/quarkus/config/datasource/DataSourceActivator.java

I would really encourage you to follow this pattern, because otherwise we'll end up with a different server module for each driver that cannot be shipped because of legal issues.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I fully agree with you @adutra

It's something we discussed in the past, and I proposed to have a tool to create custom distribution (for users), a kind of CLI.

We can imagine something like:

polaris-distribution --add-module mysql --base ...

So, I would not include a server for mysql (see my other comment) but more the module that could be use in a assembly tool.

@Y-Wakuta
Y-Wakuta force-pushed the feat/mysql-support branch 5 times, most recently from 75fc387 to 292c9d8 Compare May 7, 2026 02:46
@Y-Wakuta

Y-Wakuta commented May 7, 2026

Copy link
Copy Markdown
Author

Thanks @snazy — agreed on all framing points. A note up front: runtime/server-mysql/ was already removed earlier in this PR (per @jbonofre), so any runtime/server-mysql/... reference was stale. The driver is now opt-in via -PincludeMysqlDriver=true on runtime/server.

Changes pushed:

  1. CHANGELOG.md — rewrote along your suggested wording: "source-level support", official artifacts don't include the GPL driver, point to runtime/server/README.md in the unpacked source release.
  2. site/content/.../relational-jdbc.md — removed detailed build/properties/bootstrap from the versioned end-user docs; left a single paragraph noting it requires a custom downstream build and pointing to the source-tree README. Closing paragraph reworded to "Official artifacts
    support PostgreSQL and H2; MySQL via custom source build only".
  3. runtime/server/README.md — added a "Building a MySQL-capable server (custom downstream build)" section with the full build/config/bootstrap details, in the source-tree/developer context.
  4. No raw GitHub links remain in the versioned docs or CHANGELOG.

Let me know if any framing is still off, or if you'd prefer the source-tree README to live elsewhere (e.g. a separate BUILDING_MYSQL.md).

@Y-Wakuta

Y-Wakuta commented May 7, 2026

Copy link
Copy Markdown
Author

@adutra @jbonofre @snazy @singhpk234 @flyrain — friendly ping for another review pass when you have a moment.

The PR description has been updated to reflect the current state of the changes, and every review thread now has either a code/doc update or a follow-up reply. The constraint_name thread has two options waiting on @adutra's preference. Separately, @jbonofre — I'm keeping the custom-distribution / "polaris-distribution" tool direction you raised as a follow-up (out of scope for this PR); happy to contribute groundwork in subsequent changes if useful.

Thanks for the thorough feedback throughout.

@Y-Wakuta
Y-Wakuta requested review from adutra, flyrain and jbonofre May 7, 2026 04:04

@dimas-b dimas-b left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for your contribution, @Y-Wakuta !

Sorry, I'm coming late to the review party. I hope you do not mind 😅

Comment thread runtime/service/build.gradle.kts Outdated

// MySQL is opt-in: the GPL JDBC driver (ASF Category X) is not bundled in the default
// build. Enable with `-PincludeMysqlDriver=true` to run the MySQL integration tests.
if (project.hasProperty("includeMysqlDriver")) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's a bit odd that we have to do this both in runtime/server and runtime/service... Would one of those places be sufficient?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO the MySQL tests should run unconditionally. It's OK to have the driver in the test classpath.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 to driver on the test class path.

... but if we have this if in runtime/service, we should not need the same one in .../server... right?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks both — pushed an update: runtime/service now adds the MySQL JDBC driver unconditionally, and runtime/server consolidates into a single if/else that strips the driver by default (GPL-free) or keeps it under -PincludeMysqlDriver=true. This also fixes the CI failure where MysqlApplicationIT was hitting ClassNotFoundException: com.mysql.jdbc.Driver without the flag.

for (Map.Entry<String, Object> entry : whereEquals.entrySet()) {
conditions.add(entry.getKey() + " = ?");
if (entry.getValue() instanceof Converter.MysqlJsonValue) {
conditions.add(entry.getKey() + " = CAST(? AS JSON)");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where do we use JSON as a "where" condition?.. just wondering 🤔

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In JdbcBasePersistenceImpl.deleteFromPolicyMappingRecords — the delete's WHERE is built from ModelPolicyMappingRecord.toMap(), which includes the properties JSON column. Without CAST(? AS JSON) the bound VARCHAR doesn't match the JSON column under MySQL's comparison rules, so the delete predicate never matched after a policy detach; this was the actual cause of MysqlPolicyServiceIT.testPolicyMapping failing.

@dimas-b

dimas-b commented May 29, 2026

Copy link
Copy Markdown
Contributor

@Y-Wakuta : Please resolve conflicts to allow CI to run.

@Y-Wakuta
Y-Wakuta force-pushed the feat/mysql-support branch 2 times, most recently from dd548f9 to 745ec63 Compare May 30, 2026 13:46
dimas-b
dimas-b previously approved these changes Jun 1, 2026

@dimas-b dimas-b left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, @Y-Wakuta ! The PR LGTM 👍

Let's collect some more reviews before merging.

@dimas-b
dimas-b requested a review from snazy June 1, 2026 22:42
@dimas-b

dimas-b commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

@jbonofre : PTAL

@Y-Wakuta

Y-Wakuta commented Jun 2, 2026

Copy link
Copy Markdown
Author

Thanks for the review, @dimas-b — really appreciate it, and apologies for the delayed reply. I'll post a summary of the changes and fix the CI failures within a day or two.

@dimas-b

dimas-b commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

@Y-Wakuta : the Zizmor failure probably requires rebasing this PR to the latest main

@Y-Wakuta
Y-Wakuta force-pushed the feat/mysql-support branch 3 times, most recently from d9fcc00 to 23e5c1c Compare June 4, 2026 10:10
@Y-Wakuta

Y-Wakuta commented Jun 4, 2026

Copy link
Copy Markdown
Author

@snazy
Thank you for the careful review — this is exactly the kind of release/licensing scrutiny this change needed, and I appreciate you laying out each concern so clearly. The Gradle wiring has been restructured since the revision you looked at, largely along the lines you and @dimas-b suggested. Let me walk through each point with the current state.

  1. quarkus-jdbc-mysql on :polaris-runtime-service:runtimeClasspath

Agreed this must not leak onto a production-facing classpath. runtime/service no longer references the MySQL driver at all. The MySQL integration tests and their driver now live in a dedicated, test-only module (persistence/relational-jdbc-mysql/tests) that builds its own Quarkus server — the same pattern as extensions/auth/opa/tests. That module is not published, so mysql-connector-j stays off runtime-service's classpath and away from any downstream consumer. The default release build is GPL-free with no classpath exclusions.

  1. quarkus.datasource.mysql.jdbc=true on the normal service build

This is no longer set in runtime/service. It now applies only in runtime/server under the opt-in -PincludeMysqlDriver=true path (and inside the test-only module). The normal/release service build does not set it.

  1. License handling in runtime/server

The wholesale disabling is gone — no license tasks are disabled anymore. The default build runs generateLicenseReport / checkLicense / licenseReportZip unchanged and stays GPL-free. Under -PincludeMysqlDriver=true the build only adds the driver; the license tasks are intentionally left to fail (documented as a "verification contract"), rather than being silenced, since that flag produces a non-official downstream artifact. If you'd prefer something more targeted here — e.g. the opt-in path producing a correct license listing that includes the GPL driver rather than failing — I'm glad to take that direction; I'd value your preference on what's most appropriate for the project.

  1. README placement

Agreed — this isn't normal server documentation. The detailed build/config/bootstrap instructions have moved to separate JDBC/MySQL documentation at persistence/relational-jdbc/MYSQL.md, and runtime/server/README.md now only carries a short pointer to it, framed as an opt-in source build. The versioned site docs keep just a one-paragraph note pointing to that source-tree document, consistent with your earlier framing feedback.

Since these changes landed after your review, a fresh look whenever you have time would be very welcome. Thank you again for the thorough feedback.

@Y-Wakuta
Y-Wakuta force-pushed the feat/mysql-support branch from 23e5c1c to 6c73de5 Compare June 4, 2026 13:44
@Y-Wakuta

Y-Wakuta commented Jun 4, 2026

Copy link
Copy Markdown
Author

@dimas-b
Thanks for the pointer. you were right. I've rebased the PR onto the latest main and force-pushed, so it now picks up the latest workflow changes there. The Zizmor check should be satisfied on the next CI run. Appreciate you catching this and pointing me in the right direction.

dimas-b
dimas-b previously approved these changes Jun 4, 2026

@dimas-b dimas-b left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the update, @Y-Wakuta ! I've got a couple new minor comments (maybe I missed those aspects before 😅 )

// that fallback would otherwise surface much later as a confusing database-type mismatch.
String mysql = DatabaseType.MYSQL.getDisplayName();
if (configuredType.map(type -> type.equalsIgnoreCase(mysql)).orElse(false)) {
Instance<DataSource> mysqlDataSource = dataSources.select(new DataSourceLiteral(mysql));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: for non-MySQL datasources we select by the exact configuredType string, but here we do a case-insensitive match and use the DatabaseType.MYSQL name as the selector... why the difference?

To be clear: the case-insensitive match is fine, from my POV, but should we not use the exact configuredType string as the selector?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Subsequently, if we use configuredType as the selector in all cases, the special case-insensitive comparison may no longer be so necessary.. WDYT?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're right, the asymmetry wasn't justified. This block is new since your last review (added per a Copilot comment about failing fast when MySQL is configured but its datasource isn't resolvable). I should have flagged it explicitly, sorry. I've unified it: the named DataSource is now selected by the exact configuredType string in all cases, and the case-insensitive comparison is gone. The only spot that still references the MySQL display name is the fail-fast check, since it needs to recognize MySQL specifically (the only backend with its own named datasource) — and in the normal lowercase config it's the same string anyway.

if (configuredType.map(type -> type.equalsIgnoreCase(mysql)).orElse(false)) {
Instance<DataSource> mysqlDataSource = dataSources.select(new DataSourceLiteral(mysql));
if (!mysqlDataSource.isResolvable()) {
throw new IllegalStateException(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we do this when we fall back to the default DataSource (line 131)?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good point — done. This is part of the same block I added after your last review (per the Copilot fail-fast comment, as noted on the other thread). The fail-fast now lives in the orElseGet fallback (your line 131) instead of a separate up-front block, so a single chain handles the unresolvable case: throw for MySQL, fall back to the default otherwise.

dimas-b
dimas-b previously approved these changes Jun 5, 2026

@dimas-b dimas-b left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jbonofre : I believe your previous concern has been addressed. I hope there are no blockers on this PR now 🙂

@Y-Wakuta

Copy link
Copy Markdown
Author

Hi @jbonofre — gentle reminder when you have a moment 🙏

I believe your points are addressed:

  • No separate runtime/server-mysql — removed. The MySQL driver is opt-in on the regular runtime/server via -PincludeMysqlDriver=true; the default build stays GPL-free.
  • db-kind resolved at runtime — the named MySQL datasource is inactive by default and selected at runtime in JdbcMetaStoreManagerFactory, leaving PostgreSQL/CockroachDB/H2 untouched.
  • Custom build documented — in runtime/server/README.md and persistence/relational-jdbc/MYSQL.md.

I've also updated the PR description to match the implementation, and the MySQL integration tests pass. I think the change now meets the requirements we discussed — could you take another look when convenient? Happy to adjust anything further. Thanks again for the detailed review!

@dimas-b

dimas-b commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

@Y-Wakuta : while this PR is still under review (I hope not for much longer), please take a look at: https://lists.apache.org/thread/jy6wb186h94n9q86kv01shbn68ppr6gv

@Y-Wakuta
Y-Wakuta force-pushed the feat/mysql-support branch from 852af0a to c1b31e9 Compare July 4, 2026 16:47
@Y-Wakuta

Y-Wakuta commented Jul 6, 2026

Copy link
Copy Markdown
Author

@dimas-b
Thanks for the pointer, @dimas-b. I've read through the "[DISCUSS] Multiple datasources with runtime activation" thread.

My takeaway is that #4812's runtime datasource activation is orthogonal to this PR. Even with it, the MySQL/MariaDB drivers can't ship in an official Apache release (Connector/J is GPLv2, MariaDB's is LGPL — both Category X), so MySQL support still requires an opt-in build that adds the driver. That's exactly what this PR is designed around:

  • the default release build stays GPL-free, with no MySQL driver on the runtime-service classpath;
  • the driver and its integration tests live in a separate, unpublished test-only module;
  • the driver is only pulled in under the opt-in -PincludeMysqlDriver=true path.

So I don't think this PR conflicts with #4812, and #4812 doesn't remove the need for the opt-in build here. If #4812 lands first, I'm happy to align the MySQL datasource with its named-datasource activation mechanism (polaris.persistence.relational.jdbc.datasource=mysql) instead of relying on the build flag -- just let me know if you'd prefer I wait for that.

@dimas-b

dimas-b commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Hi @Y-Wakuta : I was hoping the "Multiple datasources" discussion would result in something that would make this PR simpler (e.g. by establishing a pattern for "dormant" datasources that only get activated at user's request)... However, in its current state, I agree that this discussion is rather orthogonal to the MySQL PR.

I hope @jbonofre can review the licenses in this PR soon and unblock it 😅

@flyrain

flyrain commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Thanks, @Y-Wakuta, for working on this. I filed PR #4984 to support runtime JDBC driver loading and dynamic datasource creation. I believe that will unblock this PR by allowing users to drop the MySQL JDBC driver into the runtime classpath, instead of requiring it to be bundled into the build. cc @jbonofre @dimas-b

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.

Support MySQL as Metastore

8 participants