dimas-b commented on code in PR #4281:
URL: https://github.com/apache/polaris/pull/4281#discussion_r3219322298
##########
runtime/service/build.gradle.kts:
##########
@@ -208,6 +206,15 @@ dependencies {
tasks.named("javadoc") { dependsOn("jandex") }
+// The MySQL JDBC driver is added unconditionally to the integration-test
runner so the
+// `*Mysql*IT` tests run on every CI build. `runtime/service` is not a
distributed artifact,
+// so bundling the GPL driver here has no licensing implication for official
Polaris
+// release artifacts (the production runner in `runtime/server` keeps the
driver opt-in).
+dependencies { runtimeOnly("io.quarkus:quarkus-jdbc-mysql") }
Review Comment:
WDYT about moving this to the bigger `dependencies` block above?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java:
##########
@@ -335,7 +336,11 @@ static QueryFragment generateWhereClauseExtended(
List<String> conditions = new ArrayList<>();
List<Object> parameters = new ArrayList<>();
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)");
Review Comment:
Having `instanceof` is not nice on the main call path. How about adding a
helper method to `DatabaseType`? For example `asJsonConditionPlaceholder(...)`?
Postgres and H2 will have a "do nothing" impl. MySQL will return `CAST(? AS
JSON)`
Subsequently, the `MysqlJsonValue` class will become unnecessary, I think 🤔
##########
runtime/service/build.gradle.kts:
##########
@@ -208,6 +206,15 @@ dependencies {
tasks.named("javadoc") { dependsOn("jandex") }
+// The MySQL JDBC driver is added unconditionally to the integration-test
runner so the
+// `*Mysql*IT` tests run on every CI build. `runtime/service` is not a
distributed artifact,
+// so bundling the GPL driver here has no licensing implication for official
Polaris
+// release artifacts (the production runner in `runtime/server` keeps the
driver opt-in).
+dependencies { runtimeOnly("io.quarkus:quarkus-jdbc-mysql") }
+// Override the `jdbc=false` default in `application.properties` so Quarkus
wires up
+// the MySQL named datasource at build time.
+quarkus { quarkusBuildProperties.put("quarkus.datasource.mysql.jdbc", "true") }
Review Comment:
Did you mean `quarkus.datasource.mysql.active`?
##########
runtime/server/build.gradle.kts:
##########
@@ -49,13 +49,29 @@ dependencies {
}
// enforce the Quarkus _platform_ here, to get a consistent and validated
set of dependencies
- implementation(enforcedPlatform(libs.quarkus.bom)) {
- exclude(group = "com.google.protobuf", module = "protobuf-java")
- exclude(group = "com.google.protobuf", module = "protobuf-java-util")
- }
+ implementation(enforcedPlatform(libs.quarkus.bom))
implementation("io.quarkus:quarkus-container-image-docker")
}
+// MySQL is opt-in: the GPL JDBC driver (ASF Category X) is not bundled in the
default
+// build. `runtime/service` adds the driver to its runtime classpath
unconditionally for
+// its own `*Mysql*IT` integration tests; here we either strip it (default
release build,
+// GPL-free) or keep it and wire it up (opt-in via
`-PincludeMysqlDriver=true`).
+if (project.hasProperty("includeMysqlDriver")) {
+ // Override the `jdbc=false` default in `application.properties` so Quarkus
wires up
+ // the MySQL named datasource at build time.
+ quarkus { quarkusBuildProperties.put("quarkus.datasource.mysql.jdbc",
"true") }
+ // The GPL driver is intentionally absent from LICENSE; skip the
license-report tasks
+ // that would otherwise fail because the driver appears on the classpath.
+ listOf("generateLicenseReport", "checkLicense", "licenseReportZip").forEach
{ name ->
+ tasks.matching { it.name == name }.configureEach { enabled = false }
+ }
+} else {
+ // Strip the MySQL JDBC driver that `runtime/service` brings in transitively
for its
+ // own integration tests; the production runner stays GPL-free.
+ configurations.all { exclude(group = "io.quarkus", module =
"quarkus-jdbc-mysql") }
Review Comment:
This may be a big change, but WDYT about moving tests (`MysqlApplicationIT`,
etc.) to a new module under `persistence`... e.g.
`persistence/relational-jdbc-mysql/tests`?
This module will build its own Quarkus server for testing (similar to
`extensions/auth/opa/tests`) with the MySQL driver and Quarkus extensions.
`runtime/service` will remain free from MySQL-related code (which is nice
from the overall design POV).
`runtime/service` will include the MySQL Quarkus plugin under a build flag,
but will not have to exclude anything if the flag is not set (cleaner
dependency tree).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
