hudi-agent commented on code in PR #19642:
URL: https://github.com/apache/hudi/pull/19642#discussion_r3786285268
##########
pom.xml:
##########
@@ -131,7 +131,13 @@
<hive.parquet.version>1.10.1</hive.parquet.version>
<hive.avro.version>1.11.4</hive.avro.version>
<presto.version>0.273</presto.version>
- <trino.version>481</trino.version>
+ <!-- trino.sha is the exact trinodb/trino commit hudi-trino builds
against; its project
+ version must equal trino.version and the two advance only together.
trino.e2e.version is
+ the latest released Trino (e2e server image, trino-jdbc for the integ
tests). Keep each
+ property on one line: the workflows and
scripts/trino/bootstrap_trino.sh read them with sed. -->
+ <trino.version>484-SNAPSHOT</trino.version>
+ <trino.sha>5b82ec9e7116ec1ed3a83f4cc2f8cf9aaa87b12f</trino.sha>
Review Comment:
🤖 Since local m2 artifacts are keyed by `trino.version` (`484-SNAPSHOT`),
advancing `trino.sha` within the same upstream dev version won't change the
artifact coordinates. Does `bootstrap_trino.sh` force a clean reinstall so a
stale `484-SNAPSHOT` from a previous pin can't be silently reused? Worth
calling out how a pin advance invalidates local m2 — CI keys its cache on the
sha, but local dev keys on the version string.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/HudiSplitSource.java:
##########
@@ -135,47 +130,32 @@ public HudiSplitSource(
queue.finish();
});
this.splitLoaderFuture =
splitLoaderExecutorService.schedule(splitLoader, 0, TimeUnit.MILLISECONDS);
- this.dynamicFilter = requireNonNull(dynamicFilter, "dynamicFilter is
null");
this.dynamicFilteringWaitTimeoutMillis =
dynamicFilteringWaitTimeoutMillis.toMillis();
- this.dynamicFilterWaitStopwatch = Stopwatch.createStarted();
}
@Override
Review Comment:
🤖 This rewrites the split-source completion semantics — the in-connector
dynamic-filter wait/poll loop and the `noMoreSplits` batch flag are gone,
replaced by an explicit `finished` flag plus
`getRequestedDynamicFilterWaitTimeoutMillis()`. Since this is a read-path
behavior change (flagged Medium risk), could you point to the tests covering
the new `isNone()` → `finished.set(true)` early-termination, and confirm the
dynamic-filter wait timeout is still honored now that the engine drives it?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
pom.xml:
##########
@@ -131,7 +131,13 @@
<hive.parquet.version>1.10.1</hive.parquet.version>
<hive.avro.version>1.11.4</hive.avro.version>
<presto.version>0.273</presto.version>
- <trino.version>481</trino.version>
+ <!-- trino.sha is the exact trinodb/trino commit hudi-trino builds
against; its project
+ version must equal trino.version and the two advance only together.
trino.e2e.version is
+ the latest released Trino (e2e server image, trino-jdbc for the integ
tests). Keep each
+ property on one line: the workflows and
scripts/trino/bootstrap_trino.sh read them with sed. -->
+ <trino.version>484-SNAPSHOT</trino.version>
+ <trino.sha>5b82ec9e7116ec1ed3a83f4cc2f8cf9aaa87b12f</trino.sha>
+ <trino.e2e.version>483</trino.e2e.version>
Review Comment:
🤖 The plugin builds at the pinned `trino.version` while the E2E server image
is `trino.e2e.version`, and the suite auto-skips during SPI drift windows.
Could you elaborate on how long drift windows are expected to last and what
validates the connector end-to-end while E2E is paused? A prolonged drift
window means no real-server coverage, which matters if a release branch is cut
during one.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
pom.xml:
##########
@@ -131,7 +131,13 @@
<hive.parquet.version>1.10.1</hive.parquet.version>
<hive.avro.version>1.11.4</hive.avro.version>
<presto.version>0.273</presto.version>
- <trino.version>481</trino.version>
+ <!-- trino.sha is the exact trinodb/trino commit hudi-trino builds
against; its project
+ version must equal trino.version and the two advance only together.
trino.e2e.version is
+ the latest released Trino (e2e server image, trino-jdbc for the integ
tests). Keep each
+ property on one line: the workflows and
scripts/trino/bootstrap_trino.sh read them with sed. -->
+ <trino.version>484-SNAPSHOT</trino.version>
Review Comment:
🤖 Tracking trino master via a pinned SHA + `-SNAPSHOT`, built from source
with no Maven Central resolution on master, is a notable shift in the
contributor/CI workflow. Could you add a short 'alternatives considered' note —
e.g. staying on released Trino with periodic incremental upgrades, or a thinner
SPI-compat shim — and why continuous drift-tracking won out? It'd also help to
state who owns advancing the pin and the expected cadence.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
docker/trino/shim/pom.xml:
##########
@@ -153,9 +154,10 @@
<scope>provided</scope>
</dependency>
- <!-- NOTE: unlike the upstream 482-SNAPSHOT shim, jts-core must NOT be
- declared provided here: it is not part of the Trino 481 SPI
surface
- and SpiDependencyChecker rejects provided scope for it. It ships
- inside the plugin dir at its transitive scope instead. -->
+ <dependency>
+ <groupId>org.locationtech.jts</groupId>
+ <artifactId>jts-core</artifactId>
Review Comment:
🤖 Flipping `jts-core` to `provided` is tied to it being on the Trino SPI
surface at this pin — the removed comment noted 481 rejected exactly this. On a
release branch that pins back to a released Trino, is jts-core guaranteed to
still be SPI-provided? The pin-back steps in `release_guide.md` don't mention
re-evaluating this scope, so a release could hit `SpiDependencyChecker`
failures.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
