wombatu-kun commented on code in PR #19642:
URL: https://github.com/apache/hudi/pull/19642#discussion_r3794096665
##########
.github/workflows/hudi_trino_e2e.yml:
##########
@@ -62,50 +64,130 @@ jobs:
# hudi-trino at HEAD, assembles the plugin dir via the in-repo shim
# (docker/trino/shim, standing in for the not-yet-released upstream
# trinodb/trino plugin/trino-hudi shim), bakes it into a local
- # apachehudi/hudi-trino_481 image, and runs ITTestTrino* against the
- # spark402 compose stack (the only pair with the trinocoordinator service).
+ # apachehudi/hudi-trino-e2e image on top of the released trino.e2e.version
+ # server, and runs ITTestTrino* against the spark402 compose stack (the
only
+ # pair with the trinocoordinator service).
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v5
+ - name: Read Trino pin
+ id: trino-pin
+ run: |
+ set -euo pipefail
+ TRINO_SHA=$(sed -n 's|.*<trino.sha>\(.*\)</trino.sha>.*|\1|p'
pom.xml)
+ TRINO_VERSION=$(sed -n
's|.*<trino.version>\(.*\)</trino.version>.*|\1|p' pom.xml)
+ E2E_VERSION=$(sed -n
's|.*<trino.e2e.version>\(.*\)</trino.e2e.version>.*|\1|p' pom.xml)
+ echo "Connector builds at $TRINO_VERSION ($TRINO_SHA); server image
is $E2E_VERSION"
+ echo "trino_sha=$TRINO_SHA" >> "$GITHUB_OUTPUT"
+ echo "trino_version=$TRINO_VERSION" >> "$GITHUB_OUTPUT"
+ echo "e2e_version=$E2E_VERSION" >> "$GITHUB_OUTPUT"
+ - name: SPI drift gate
+ id: spi-drift
+ # The plugin is built at the pin but loaded by the released
trino.e2e.version server, so
+ # any SPI / filesystem change between the two can make the image
unbootable. Skip the run
+ # instead of reporting a failure that no connector change caused.
+ env:
+ GH_TOKEN: ${{ github.token }}
+ TRINO_SHA: ${{ steps.trino-pin.outputs.trino_sha }}
+ TRINO_VERSION: ${{ steps.trino-pin.outputs.trino_version }}
+ E2E_VERSION: ${{ steps.trino-pin.outputs.e2e_version }}
+ run: |
+ set -euo pipefail
+ # Per-path commit queries, NOT the compare API: compare caps its
file list at 300
+ # and a single Trino release cycle already exceeds that, so a capped
compare would
+ # flag every pin more than a release old as drifted. The commits API
is uncapped;
+ # any commit reachable from the pin that touched an SPI-relevant
path after the
+ # released tag's commit date (excluding the tag commit itself) is
drift. Existence
+ # is enough, so the first page settles it -- truncation cannot yield
a false pass.
+ TAG_SHA=$(gh api "repos/trinodb/trino/commits/${E2E_VERSION}" --jq
.sha)
+ TAG_DATE=$(gh api "repos/trinodb/trino/commits/${E2E_VERSION}" --jq
.commit.committer.date)
+ DRIFTED=false
+ for p in core/trino-spi lib/trino-filesystem
lib/trino-filesystem-manager lib/trino-hdfs; do
Review Comment:
The shim marks only `trino-spi`, `slice`, `jackson-annotations`,
`opentelemetry` and `jts-core` as `provided`, matching `PluginManager`'s SPI
package list, so the filesystem libraries ship inside the plugin dir and a
change under them cannot make the image unbootable as the step comment says. Is
the wider net deliberate, or should the gate key on `core/trino-spi` alone so
drift windows close sooner?
##########
docker/trino/build_image.sh:
##########
@@ -15,21 +15,21 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-# Builds the apachehudi/hudi-trino_<version> image with a locally-built
-# trino-hudi plugin baked in. The plugin dir (typically the in-repo shim's
+# Builds the apachehudi/hudi-trino-e2e image with a locally-built trino-hudi
+# plugin baked in. The plugin dir (typically the in-repo shim's
# docker/trino/shim/target/trino-hudi-<v>, see docker/trino/shim/pom.xml) is
# staged into the build context at docker/trino/plugin/ (gitignored), then
# baked into the image.
# Usage: ./build_image.sh --plugin-dir <path> [--trino-version <v>]
[--image-tag <t>]
-# Typical: ./build_image.sh --plugin-dir "$(dirname
"$0")/shim/target/trino-hudi-481"
-# Note: --trino-version must match the shim pom's parent version and the root
-# pom's trino.version property.
+# Typical: ./build_image.sh --plugin-dir "$(dirname
"$0")/shim/target/trino-hudi-<trino.version>"
+# Note: --trino-version is the released Trino server image to build on top of
+# (the root pom's trino.e2e.version), not the version the plugin was built at.
set -e
# Default values
PLUGIN_DIR=""
-TRINO_VERSION="481"
+TRINO_VERSION="483"
Review Comment:
The nightly `propose-pin-advance` job seds `trino.e2e.version` in the root
pom only, so this default and the `Dockerfile` `ARG` keep their literal the
first time that version moves, and `hudi-trino/README.md`'s claim that the base
server defaults to `trino.e2e.version` stops holding. Read the default from the
root pom with the same sed the workflows use, or have the propose job rewrite
both literals too.
##########
.github/workflows/hudi_trino_e2e.yml:
##########
@@ -62,50 +64,130 @@ jobs:
# hudi-trino at HEAD, assembles the plugin dir via the in-repo shim
# (docker/trino/shim, standing in for the not-yet-released upstream
# trinodb/trino plugin/trino-hudi shim), bakes it into a local
- # apachehudi/hudi-trino_481 image, and runs ITTestTrino* against the
- # spark402 compose stack (the only pair with the trinocoordinator service).
+ # apachehudi/hudi-trino-e2e image on top of the released trino.e2e.version
+ # server, and runs ITTestTrino* against the spark402 compose stack (the
only
+ # pair with the trinocoordinator service).
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v5
+ - name: Read Trino pin
+ id: trino-pin
+ run: |
+ set -euo pipefail
+ TRINO_SHA=$(sed -n 's|.*<trino.sha>\(.*\)</trino.sha>.*|\1|p'
pom.xml)
+ TRINO_VERSION=$(sed -n
's|.*<trino.version>\(.*\)</trino.version>.*|\1|p' pom.xml)
+ E2E_VERSION=$(sed -n
's|.*<trino.e2e.version>\(.*\)</trino.e2e.version>.*|\1|p' pom.xml)
+ echo "Connector builds at $TRINO_VERSION ($TRINO_SHA); server image
is $E2E_VERSION"
+ echo "trino_sha=$TRINO_SHA" >> "$GITHUB_OUTPUT"
+ echo "trino_version=$TRINO_VERSION" >> "$GITHUB_OUTPUT"
+ echo "e2e_version=$E2E_VERSION" >> "$GITHUB_OUTPUT"
+ - name: SPI drift gate
+ id: spi-drift
+ # The plugin is built at the pin but loaded by the released
trino.e2e.version server, so
+ # any SPI / filesystem change between the two can make the image
unbootable. Skip the run
+ # instead of reporting a failure that no connector change caused.
+ env:
+ GH_TOKEN: ${{ github.token }}
+ TRINO_SHA: ${{ steps.trino-pin.outputs.trino_sha }}
+ TRINO_VERSION: ${{ steps.trino-pin.outputs.trino_version }}
+ E2E_VERSION: ${{ steps.trino-pin.outputs.e2e_version }}
+ run: |
+ set -euo pipefail
+ # Per-path commit queries, NOT the compare API: compare caps its
file list at 300
+ # and a single Trino release cycle already exceeds that, so a capped
compare would
+ # flag every pin more than a release old as drifted. The commits API
is uncapped;
+ # any commit reachable from the pin that touched an SPI-relevant
path after the
+ # released tag's commit date (excluding the tag commit itself) is
drift. Existence
+ # is enough, so the first page settles it -- truncation cannot yield
a false pass.
+ TAG_SHA=$(gh api "repos/trinodb/trino/commits/${E2E_VERSION}" --jq
.sha)
+ TAG_DATE=$(gh api "repos/trinodb/trino/commits/${E2E_VERSION}" --jq
.commit.committer.date)
+ DRIFTED=false
+ for p in core/trino-spi lib/trino-filesystem
lib/trino-filesystem-manager lib/trino-hdfs; do
+ for c in $(gh api
"repos/trinodb/trino/commits?sha=${TRINO_SHA}&path=${p}&since=${TAG_DATE}" --jq
'.[].sha'); do
Review Comment:
A failing command substitution in a `for` list is not caught by `set -e`,
and `gh api` prints its error body to stdout, so a transient 403 or 502
iterates over the error JSON, sets `drifted=true` and skips the suite with the
job still green. Assign each response to a variable first so the failure aborts
the step.
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/HudiSplitSource.java:
##########
@@ -188,7 +168,13 @@ public void close()
@Override
public boolean isFinished()
{
- return splitLoaderFuture.isDone() && queue.isFinished();
+ return finished.get() || (splitLoaderFuture.isDone() &&
queue.isFinished());
Review Comment:
`isFinished()` ignores `trinoException`, so once the failure handler's
`queue.finish()` lets the draining batch report finished, the engine closes the
source and never polls again, and a split-loading failure ends the scan
silently instead of failing the query. Gate `isFinished()` on
`trinoException.get() == null` so the next `getNextBatch` still reports it.
##########
hudi-trino/src/test/java/io/trino/plugin/hudi/TestHudiSmokeTest.java:
##########
@@ -928,6 +937,26 @@ public void
testDynamicFilterEnabledPredicatePushdown(ResourceHudiTablesInitiali
assertQuery(query, "VALUES (1, 'a1', 100.0, 1000), (3, 'a3', 101.0,
1001)");
}
+ @ParameterizedTest
+ @EnumSource(
+ value = ResourceHudiTablesInitializer.TestingTable.class,
+ names = {"HUDI_MULTI_FG_PT_V6_MOR", "HUDI_MULTI_FG_PT_V8_MOR"})
+ public void
testDynamicFilterEliminatesAllSplits(ResourceHudiTablesInitializer.TestingTable
table)
+ {
+ Session session = SessionBuilder
+ .from(getSession())
+ .withDynamicFilterTimeout("10s")
+ .build();
+
+ // The build side matches no rows, so the completed dynamic filter is
NONE and the
+ // probe-side split source must report itself finished instead of
draining the queue
+ @Language("SQL") String query = "SELECT t1.id FROM " +
+ table + " t1 " +
+ "INNER JOIN " + table + " t2 ON t1.id = t2.id " +
+ "WHERE t2.price < 0";
+ assertThat(getQueryRunner().execute(session,
query).getRowCount()).isEqualTo(0);
Review Comment:
An inner join whose build side matches nothing returns no rows whether or
not the split source terminates early, so this assertion does not separate the
new `finished` path from draining and filtering every split. Is there a signal
you can assert instead, the way `testDynamicFilterEnabledPredicatePushdown`
reads the probe-side `Input: N rows` out of `EXPLAIN ANALYZE`?
##########
release/release_guide.md:
##########
@@ -290,6 +290,31 @@ Here is how to go about a bug fix release.
- Go to apache/hudi repo locally and pull this branch. Here after you can work
on this branch and push to origin when need be.
- Do not forget to set the env variables from above section.
+## hudi-trino Trino pin-back
+
+On master hudi-trino tracks `trinodb/trino` master at the commit in
`trino.sha`, whose `trino.version` is a
+`-SNAPSHOT` that resolves from nowhere but a local build. A release must
depend on a released Trino, and the pin-back
+must land on the release branch before the source release is generated (see
"Build a release candidate", the Generate
+Source Release step) -- otherwise the voted tarball ships a `-SNAPSHOT` Trino
pin that cannot be built from Central.
+
+1. Wait for the latest released Trino `NNN` to be available on Maven Central.
+2. In a `trinodb/trino` checkout, find the tagged commit: `TAG_SHA=$(git
rev-list -n1 NNN)`.
+3. If the pin is behind the tag, advance master's pin to `TAG_SHA` first by
running the
+ `Hudi Trino SPI Compatibility` workflow via `workflow_dispatch` and merging
the pin PR a committer opens from the
Review Comment:
`hudi_trino_compat.yml` checks out `trinodb/trino` at a hardcoded `master`
and its `workflow_dispatch` takes no inputs, so running it cannot advance the
pin to `TAG_SHA`; it pins to whatever master HEAD is at that moment. Drop that
sentence and let step 4 set `trino.sha=TAG_SHA` on the release branch, or add a
ref input to the workflow.
--
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]
