You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@xtable.apache.org by "rangareddy (via GitHub)" <gi...@apache.org> on 2026/07/21 13:32:27 UTC
[PR] [727] Fix demo notebook to run against current main [incubator-xtable]
rangareddy opened a new pull request, #850:
URL: https://github.com/apache/incubator-xtable/pull/850
## What is the purpose of the pull request
The demo notebook (`demo/notebook/demo.ipynb`) no longer runs on current `main`. This PR fixes all the issues reported in #727 plus three more found while getting it running end-to-end.
Closes #727
## Changes
**`demo/notebook/demo.ipynb`**
1. **Jar coordinate**: `xtable-core-0.2.0-SNAPSHOT.jar` → `xtable-core_2.12-0.2.0-SNAPSHOT.jar` (artifact gained the Scala binary version suffix).
2. **API drift**: ported all sync cells from the removed `PerTableConfigImpl` (deleted in #480) to the current `SourceTable`/`TargetTable`/`ConversionConfig` builders, and `ConversionSourceProvider#init` to its single-`Configuration` signature. Explicit empty `Properties` are passed on the builders because `HudiSourceConfig.fromProperties` throws an NPE when `additionalProperties` is null.
3. **Dependency versions**: Hudi `0.14.0` → `1.2.0` (matches #772) and `iceberg-hive-runtime` `1.4.2` → `1.7.2` (the version pinned in the root pom). Added `delta-kernel-api`/`delta-kernel-defaults` `4.0.0` — they are `provided`-scope dependencies of xtable-core, and without them the `ConversionTarget` ServiceLoader fails on `DeltaKernelConversionTarget` before any sync can run.
4. **Removed the `hoodie.write.schema` override**: under Hudi 1.2's lazy record serialization the rows are encoded with the nullable dataframe-derived schema but were decoded with the non-null override schema, so every record failed with `AvroRuntimeException: Malformed data. Length is negative`. With the override removed one consistent schema is used.
**`demo/docker-compose.yaml` + `demo/jupyter/setup-jdk11.sh` (new)**
Hudi 1.x classes are Java 11 bytecode, but the `almondsh/almond` image only ships Java 8 (there is no JDK 11 tag), which fails with `UnsupportedClassVersionError`. The new `before-notebook.d` startup hook installs OpenJDK 11 via mamba on first container start and points the Scala kernels at it.
## Verification
Executed every cell of the notebook end-to-end against a freshly recreated jupyter container (so the JDK 11 hook provisioning is exercised from scratch) with pristine seed data:
- Hudi → Iceberg sync + incremental re-sync after appending round-2 CSV
- Trino `INSERT` into the Iceberg table, then the new row read back **as Delta** via Spark
- Delta → Hudi sync and read-back as Hudi
- Cross-format Delta join of the two tables
- Delta → Iceberg sync
Final state via Trino: `iceberg.demo.hudi_dimCustomer` = 5001 rows, `iceberg.demo.delta_dimGeography` = 674 rows.
Note for reviewers: on first container start the hook downloads OpenJDK 11 (~200 MB, one-time); subsequent starts are instant.
## Verify this pull request
This pull request is a trivial rework / code cleanup without any test coverage.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [727] Fix demo notebook to run against current main [incubator-xtable]
Posted by "vinishjail97 (via GitHub)" <gi...@apache.org>.
vinishjail97 commented on code in PR #850:
URL: https://github.com/apache/incubator-xtable/pull/850#discussion_r3716212904
##########
demo/jupyter/prefetch_dependencies.sh:
##########
@@ -0,0 +1,52 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+## Pre-fetch the demo notebook dependencies into the coursier cache at image
+## build time so the notebooks' first cell resolves instantly. The dependency
+## versions are read from the versions.properties file passed as the first
+## argument (generated from the root pom by build_demo.sh).
+set -e
+
+PROPS_FILE="$1"
+
+prop() {
+ grep "^$1=" "${PROPS_FILE}" | cut -d= -f2
+}
+
+SCALA_BINARY_VERSION=$(prop scala.binary.version)
+SPARK_VERSION=$(prop spark.version)
Review Comment:
`prop()` returns an empty string for a missing or renamed key, with no error. So a typo — or `build_demo.sh` dropping a property from its loop at line 49 — produces `org.apache.spark:spark-sql_:` and a coursier resolution error that says nothing about the real cause. Since `set -e` is on, the image build fails at that point with a confusing message.
`predef.sc:33-36` gets this right (`sys.error(s"$key missing from ...")`). Mirror it:
```bash
prop() {
local value
value=$(grep "^$1=" "${PROPS_FILE}" | cut -d= -f2-)
if [ -z "${value}" ]; then
echo "ERROR: $1 missing from ${PROPS_FILE}; re-run build_demo.sh" >&2
exit 1
fi
echo "${value}"
}
```
Note `-f2-` rather than `-f2` — the current version truncates any value containing `=`. None do today, but it is free to be correct.
##########
demo/jupyter/predef.sc:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// Kernel predef for the XTable demo notebooks: loads every dependency the
+// notebooks need when the kernel starts, so the notebooks themselves contain
+// no dependency management. The artifacts are already present in the image's
+// coursier cache (pre-fetched at image build time); versions come from
+// /home/jars/versions.properties, generated from the root pom by
+// build_demo.sh.
+
+val xtableVersions = {
+ val props = new java.util.Properties()
+ val in = new java.io.FileInputStream("/home/jars/versions.properties")
+ try props.load(in)
+ finally in.close()
+ props
+}
+
+def xtableVersion(key: String): String =
+ Option(xtableVersions.getProperty(key)).map(_.trim).filter(_.nonEmpty).getOrElse {
+ sys.error(s"$key missing from /home/jars/versions.properties, re-run build_demo.sh")
+ }
+
+{
+ val scalaBinaryVersion = xtableVersion("scala.binary.version")
+ val sparkVersionPrefix = xtableVersion("spark.version.prefix")
+
+ Seq(
+ ("org.apache.logging.log4j", "log4j-api", "2.17.2"),
Review Comment:
log4j is pinned to 2.17.2 here while the project itself builds against 2.22.0 (`org.apache.logging.log4j:log4j-api:2.22.0` in the shaded modules' `LICENSE-bundled`). Given the surrounding change deliberately moves Hudi and Iceberg onto the versions the root pom pins, was the older log4j intentional — something in the almond/Spark stack that needs it — or just carried over from the original notebook?
If there is no reason for it, reading `log4j.version` from `versions.properties` like everything else keeps the demo consistent with what XTable is actually tested against.
##########
demo/docker-compose.yaml:
##########
@@ -54,7 +55,12 @@ services:
jupyter:
container_name: jupyter
hostname: jupyter
- image: 'almondsh/almond:latest'
+ build:
+ context: .
+ dockerfile: jupyter/Dockerfile
+ image: 'xtable-demo-jupyter'
+ ## Local demo only: start the notebook server without authentication.
+ command: ["start-notebook.sh", "--IdentityProvider.token=", "--ServerApp.password="]
Review Comment:
This disables authentication on the notebook server, and line 65 publishes it as `'8888:8888'` — bound to every interface on the host.
Combined, anyone who can reach the machine on port 8888 (same office wifi, same k8s node, same VPN segment) gets an unauthenticated Scala kernel with arbitrary code execution and read/write access to the mounted `demo/data`. The `## Local demo only` comment states the intent but nothing enforces it, and the previous behaviour (token in `docker logs`) was safe by default.
At minimum bind to loopback:
```yaml
ports:
- '127.0.0.1:8888:8888'
```
That keeps the zero-friction "open the printed URL" experience the PR is going for — `start_demo.sh` already prints `http://localhost:8888/lab` — while making the exposure actually local. If you would rather keep the token, `start_demo.sh` could grep it out of `docker logs` and print the full URL, which is the same UX and needs no auth change at all.
Worth noting the other three services (lines 22, 32, 47) have the same all-interfaces binding, but those are pre-existing and not unauthenticated code execution.
##########
demo/jupyter/Dockerfile:
##########
@@ -0,0 +1,52 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Jupyter image for the XTable demo, based on the almond Scala kernel image.
+# Everything the notebooks need at runtime is baked in at build time so the
+# first cell does not spend minutes provisioning:
+# - OpenJDK 11 for the Scala kernels (Hudi 1.x jars require Java 11+)
+# - all notebook dependencies (Spark, Hudi, Delta, Iceberg, JDBC drivers)
+# pre-fetched into the coursier cache
+# - the Scala kernels re-pointed at that JDK, with a bounded heap and a
+# predef that puts every dependency on the classpath at kernel startup
+FROM almondsh/almond:latest
Review Comment:
`:latest` on the base image, in a PR whose whole purpose is that the demo stopped working because things moved underneath it.
That matters more than usual here because of the kernel-patching step at the bottom of this file: it globs `/home/jovyan/.local/share/jupyter/kernels/scala*/kernel.json`, replaces `argv[0]` positionally, and inserts `-Xmx4g` at index 1. All three assumptions belong to whatever `almondsh/almond:latest` is on the day the image is built. When the tag moves — a different kernel path, a different `argv` shape, a jovyan→different user change — the glob matches nothing, the `for` comprehension iterates over an empty list, `print('patched kernels:', [])` succeeds, and the build passes with a broken image. Someone will then reopen #727.
Pin it. `almondsh/almond` publishes versioned tags (`0.14.0-scala-2.12.17` and similar); a digest pin (`FROM almondsh/almond@sha256:...`) is stronger still. And make the patch step assert rather than no-op:
```python
assert paths, 'no scala kernels found under ~/.local/share/jupyter/kernels'
```
Same reasoning applies to `image: 'xtable-demo-jupyter'` in the compose file — an untagged local image means `:latest`, so a stale build from a previous checkout gets reused silently unless `--build` is passed (which `start_demo.sh` does, so that one is fine in practice).
##########
demo/jupyter/prefetch_dependencies.sh:
##########
@@ -0,0 +1,52 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+## Pre-fetch the demo notebook dependencies into the coursier cache at image
+## build time so the notebooks' first cell resolves instantly. The dependency
+## versions are read from the versions.properties file passed as the first
+## argument (generated from the root pom by build_demo.sh).
+set -e
+
+PROPS_FILE="$1"
+
+prop() {
+ grep "^$1=" "${PROPS_FILE}" | cut -d= -f2
+}
+
+SCALA_BINARY_VERSION=$(prop scala.binary.version)
+SPARK_VERSION=$(prop spark.version)
+SPARK_VERSION_PREFIX=$(prop spark.version.prefix)
+HUDI_VERSION=$(prop hudi.version)
+DELTA_VERSION=$(prop delta.version)
+DELTA_KERNEL_VERSION=$(prop delta.kernel.version)
+ICEBERG_HIVE_RUNTIME_VERSION=$(prop iceberg.hive.runtime.version)
+
+coursier fetch --quiet \
+ "org.apache.logging.log4j:log4j-api:2.17.2" \
Review Comment:
These four coordinates — log4j 2.17.2 (×2), `io.trino:trino-jdbc:431`, `com.facebook.presto:presto-jdbc:0.283` — are hardcoded here *and* again in `predef.sc:44-55`. That is the exact duplication `versions.properties` was introduced to eliminate; the other eight coordinates correctly read from it, these four do not.
The drift is silent and has a real cost. The prefetch list is what warms the coursier cache at image build time; the predef list is what the kernel resolves at startup. If they diverge by one version, the predef falls through to a network resolve on every kernel start — which is precisely the slow path the Dockerfile comment says this design exists to avoid ("so the first cell does not spend minutes provisioning"), and it will look like a flaky demo rather than a version mismatch.
Put them in `versions.properties` alongside the rest. `build_demo.sh:49` already loops over a property list, so it is a matter of adding `log4j.version`, `trino.jdbc.version` and `presto.jdbc.version` — the first can come from the root pom like the others, and the last two can be literals in `build_demo.sh` if there is no pom property for them. Either way there is then one place to change a version.
--
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: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [727] Fix demo notebook to run against current main [incubator-xtable]
Posted by "rangareddy (via GitHub)" <gi...@apache.org>.
rangareddy commented on PR #850:
URL: https://github.com/apache/incubator-xtable/pull/850#issuecomment-5190038652
Rebuilt as a single commit `719ad4a` (the `scala.Array(namespace)` fix folded in — `java.util._` is imported, so a bare `Array` resolves against `java.lang.reflect.Array` under Ammonite). Squash verified content-preserving: `git diff` against the merge base has the same checksum before and after.
**I need to retract a claim in the description.** It said every cell had been executed end-to-end on pristine seed data. That is not true on the current revision, and it was wrong rather than merely outdated, so I have corrected the description rather than quietly editing around it.
The round-2 append cell fails **deterministically** on a freshly reset `demo/data`:
```
ERROR TableFormatSync: Failed to sync table changes
java.lang.IllegalArgumentException: Cannot add field CustomerKey as an identifier field: not a required field
at org.apache.iceberg.Schema.validateIdentifierField(Schema.java:159)
at org.apache.iceberg.SchemaUpdate.applyChanges(SchemaUpdate.java:554)
at org.apache.xtable.iceberg.IcebergSchemaSync.sync(IcebergSchemaSync.java:60)
```
| step | `CustomerKey` |
|---|---|
| tracked seed commit `20231105172136406.commit` | `"string"` → **required** |
| initial sync → Iceberg metadata v1 | `identifier-field-ids=[2]`, `required=true` |
| round-2 append commit | `["null","string"]` → **optional** |
| incremental sync | fails as above |
The round-2 CSV is read with `header=true` and no schema, so Spark types every column as a nullable string, flipping `CustomerKey` from required to optional. `IcebergSchemaExtractor.toIceberg` then correctly declines to set identifier fields for a nullable record key — that is the guard added for #366 — so XTable's target schema has none. But the **table** still carries `identifier-field-ids=[2]` from the initial sync, and `SchemaUpdate.apply()` re-validates those against the new schema.
`grep -rn 'setIdentifierFields' xtable-core/src/main/java` returns nothing: `IcebergSchemaSync` never reconciles identifier fields, so once a record key goes required → optional the Iceberg target is permanently wedged — every subsequent sync fails the same way.
This is a **distinct gap from #261 / #366**, which fixed table *creation* with an already-nullable key rather than the transition on an existing table. It reproduces outside the demo, so it is an XTable bug that the demo merely surfaces — deterministically, because the seed's required key plus a schema-less round-2 read guarantees the transition, and `stop_demo.sh --reset-data` restores exactly that state.
Why it looked green before: a `demo/data` directory carried over from a previous run already has `CustomerKey` nullable, so the initial sync never sets an identifier field and the round-2 cell passes. Order-dependent, which is why neither of us caught it.
Two things I would like your view on before this merges:
1. **How should `IcebergSchemaSync` handle it** — clear the identifier fields when the latest schema has none, refuse the nullability change with a clear error, or keep the record key required on the Iceberg side? That is a semantics decision, so I would rather not pick it unilaterally. Happy to file it as an issue with this evidence and take it to `dev@` if you prefer.
2. **Whether this PR should merge before that is fixed.** The round-2 cell is part of the notebook, so as it stands a new user following the demo hits the failure. Everything else in the notebook passes; the hardening changes are independent of it.
For the record, `inferSchema=true` on the round-2 read is not a workaround: the existing schema is all required strings, so inference additionally gives `CustomerKey: int` and `YearlyIncome: double`, adding a string → int change that Iceberg rejects outright.
--
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: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [727] Fix demo notebook to run against current main [incubator-xtable]
Posted by "vinishjail97 (via GitHub)" <gi...@apache.org>.
vinishjail97 merged PR #850:
URL: https://github.com/apache/incubator-xtable/pull/850
--
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: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [727] Fix demo notebook to run against current main [incubator-xtable]
Posted by "vinishjail97 (via GitHub)" <gi...@apache.org>.
vinishjail97 commented on PR #850:
URL: https://github.com/apache/incubator-xtable/pull/850#issuecomment-5186034158
@rangareddy Thanks for working on this! I have validated the demo notebooks locally and added few comments on making the demo more robust.
--
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: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [727] Fix demo notebook to run against current main [incubator-xtable]
Posted by "rangareddy (via GitHub)" <gi...@apache.org>.
rangareddy commented on code in PR #850:
URL: https://github.com/apache/incubator-xtable/pull/850#discussion_r3718505365
##########
demo/docker-compose.yaml:
##########
@@ -54,7 +55,12 @@ services:
jupyter:
container_name: jupyter
hostname: jupyter
- image: 'almondsh/almond:latest'
+ build:
+ context: .
+ dockerfile: jupyter/Dockerfile
+ image: 'xtable-demo-jupyter'
+ ## Local demo only: start the notebook server without authentication.
+ command: ["start-notebook.sh", "--IdentityProvider.token=", "--ServerApp.password="]
Review Comment:
Fixed — bound to loopback, as you suggested:
```yaml
ports:
- '127.0.0.1:8888:8888'
```
I took this over the keep-the-token alternative: it removes the reachability outright rather than relying on a secret, adds no new failure mode, and `http://localhost:8888/lab` keeps working exactly as `start_demo.sh` already prints it. I also rewrote the comment so it says what the binding is protecting rather than only stating intent — the previous `## Local demo only` was the thing that wasn't enforcing anything.
Verified against the running container rather than just reading the config:
```
$ docker port jupyter
8888/tcp -> 127.0.0.1:8888
$ netstat -an | grep '\.8888'
tcp4 0 0 127.0.0.1.8888 *.* LISTEN
$ curl -o /dev/null -w '%{http_code}\n' http://127.0.0.1:8888/lab
200
$ curl -o /dev/null -w '%{http_code}\n' --max-time 6 http://10.3.232.82:8888/lab # host LAN address
000 # refused
$ docker port trino # untouched, for contrast
8080/tcp -> 0.0.0.0:8080
```
Left the other three services as they are, per your note that they are pre-existing and not unauthenticated code execution.
##########
demo/jupyter/Dockerfile:
##########
@@ -0,0 +1,52 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Jupyter image for the XTable demo, based on the almond Scala kernel image.
+# Everything the notebooks need at runtime is baked in at build time so the
+# first cell does not spend minutes provisioning:
+# - OpenJDK 11 for the Scala kernels (Hudi 1.x jars require Java 11+)
+# - all notebook dependencies (Spark, Hudi, Delta, Iceberg, JDBC drivers)
+# pre-fetched into the coursier cache
+# - the Scala kernels re-pointed at that JDK, with a bounded heap and a
+# predef that puts every dependency on the classpath at kernel startup
+FROM almondsh/almond:latest
Review Comment:
Both done.
**Pinned by tag *and* digest**, since a tag can be re-pushed:
```dockerfile
FROM almondsh/almond:0.14.1@sha256:0407359be58529eaa8e35bcf4586c3a890123be64005600b381de88fdc3c5de7
```
Worth noting this pin is a **no-op behaviourally**: Docker Hub reports `latest` and `0.14.1` with the *same* manifest digest (`sha256:0407359be5…`, last updated 2025-02-25). So it changes nothing today and just stops the tag moving underneath us.
One correction to your suggestion: `0.14.0-scala-2.12.17` does not exist. The scala-suffixed tags are per-Scala *single-kernel* images (`0.14.1-scala-2.12.20`, `-2.13.16`, `-3.6.3`); the unsuffixed `0.14.1` is the multi-kernel image that the `scala*` glob in this file is written for, so that is the correct one to pin.
**Asserts** — I added two rather than one, because you named three assumptions and `assert paths` only covers the first:
```python
paths = sorted(glob.glob('.../kernels/scala*/kernel.json'))
assert paths, 'no scala kernels found under /home/jovyan/.local/share/jupyter/kernels'
kernels = [(p, json.load(open(p))) for p in paths]
assert all(k['argv'][0] == 'java' for _, k in kernels), 'kernel argv[0] is not java: %r' % [...]
```
If `argv[0]` were ever a wrapper script rather than the JVM, replacing index 0 and inserting `-Xmx4g` at index 1 would produce a broken kernel that the glob assert would happily pass. `sorted()` also makes the patch order deterministic.
Verified the assumptions actually hold for the pinned image:
```
/home/jovyan/.local/share/jupyter/kernels/{scala212,scala213,scala36}/
argv[0] == "java", user == jovyan, base java == 1.8.0_442 (so the JDK 11 install is indeed needed)
```
and that the asserts fail rather than no-op when broken:
```
AssertionError: no scala kernels found under /home/jovyan/.local/share/jupyter/kernels
AssertionError: kernel argv[0] is not java: ['python']
```
The real build then reports `patched kernels: [scala212, scala213, scala36]`.
One thing you may want to know separately: `almondsh/almond` publishes **amd64 only** — there is no arm64 variant, so on Apple Silicon the whole demo runs under emulation. Pre-existing, but it makes the demo noticeably slow on M-series machines, and it is why I could not get a throwaway Hive Metastore through derby schema init locally.
##########
demo/jupyter/prefetch_dependencies.sh:
##########
@@ -0,0 +1,52 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+## Pre-fetch the demo notebook dependencies into the coursier cache at image
+## build time so the notebooks' first cell resolves instantly. The dependency
+## versions are read from the versions.properties file passed as the first
+## argument (generated from the root pom by build_demo.sh).
+set -e
+
+PROPS_FILE="$1"
+
+prop() {
+ grep "^$1=" "${PROPS_FILE}" | cut -d= -f2
+}
+
+SCALA_BINARY_VERSION=$(prop scala.binary.version)
+SPARK_VERSION=$(prop spark.version)
Review Comment:
Applied exactly as written, including `cut -d= -f2-`.
Verified both halves:
```
$ bash demo/jupyter/prefetch_dependencies.sh <props file missing log4j.version>
ERROR: log4j.version missing from …/props-incomplete.properties; re-run build_demo.sh
# and the -f2- fix
weird.key -> a=b=c (old cut -f2 gave: a)
```
This turned out to pay for itself immediately: because the change below adds three new keys, a stale `demo/jars/versions.properties` from a build predating it now stops the container at the startup gate naming the missing key, instead of failing later inside coursier.
##########
demo/jupyter/prefetch_dependencies.sh:
##########
@@ -0,0 +1,52 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+## Pre-fetch the demo notebook dependencies into the coursier cache at image
+## build time so the notebooks' first cell resolves instantly. The dependency
+## versions are read from the versions.properties file passed as the first
+## argument (generated from the root pom by build_demo.sh).
+set -e
+
+PROPS_FILE="$1"
+
+prop() {
+ grep "^$1=" "${PROPS_FILE}" | cut -d= -f2
+}
+
+SCALA_BINARY_VERSION=$(prop scala.binary.version)
+SPARK_VERSION=$(prop spark.version)
+SPARK_VERSION_PREFIX=$(prop spark.version.prefix)
+HUDI_VERSION=$(prop hudi.version)
+DELTA_VERSION=$(prop delta.version)
+DELTA_KERNEL_VERSION=$(prop delta.kernel.version)
+ICEBERG_HIVE_RUNTIME_VERSION=$(prop iceberg.hive.runtime.version)
+
+coursier fetch --quiet \
+ "org.apache.logging.log4j:log4j-api:2.17.2" \
Review Comment:
Done — all four now come from `versions.properties`:
- `build_demo.sh` adds `log4j.version` to the pom-property loop, and declares `TRINO_JDBC_VERSION` / `PRESTO_JDBC_VERSION` as literals with a comment tying them to the trino/presto image tags in `docker-compose.yaml` (as you suggested — there is no root-pom property because xtable does not depend on either driver).
- `prefetch_dependencies.sh` and `predef.sc` both read all three.
- Both lists now carry a comment explaining they must resolve to the same artifacts, and why: cache-warm at build time vs resolve at kernel start.
No coordinate in either file states a version any more. `verify_dependencies.sh` needed no change since it delegates to `prefetch_dependencies.sh`.
`./demo/build_demo.sh` → `BUILD SUCCESS`, and `versions.properties` now has all ten keys:
```
scala.binary.version=2.12 delta.kernel.version=4.0.0
spark.version=3.4.2 iceberg.hive.runtime.version=1.7.2
spark.version.prefix=3.4 log4j.version=2.22.0
hudi.version=1.2.0 trino.jdbc.version=431
delta.version=2.4.0 presto.jdbc.version=0.283
```
One thing I noticed once they were side by side and did **not** change: `docker-compose.yaml` runs **trino 428** while the driver is **trino-jdbc 431** (presto matches at 0.283). The Trino JDBC driver is version-tolerant and you validated the notebooks on 431, so I left it — but they are now in one place if you want them aligned.
##########
demo/jupyter/predef.sc:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// Kernel predef for the XTable demo notebooks: loads every dependency the
+// notebooks need when the kernel starts, so the notebooks themselves contain
+// no dependency management. The artifacts are already present in the image's
+// coursier cache (pre-fetched at image build time); versions come from
+// /home/jars/versions.properties, generated from the root pom by
+// build_demo.sh.
+
+val xtableVersions = {
+ val props = new java.util.Properties()
+ val in = new java.io.FileInputStream("/home/jars/versions.properties")
+ try props.load(in)
+ finally in.close()
+ props
+}
+
+def xtableVersion(key: String): String =
+ Option(xtableVersions.getProperty(key)).map(_.trim).filter(_.nonEmpty).getOrElse {
+ sys.error(s"$key missing from /home/jars/versions.properties, re-run build_demo.sh")
+ }
+
+{
+ val scalaBinaryVersion = xtableVersion("scala.binary.version")
+ val sparkVersionPrefix = xtableVersion("spark.version.prefix")
+
+ Seq(
+ ("org.apache.logging.log4j", "log4j-api", "2.17.2"),
Review Comment:
Carried over — not intentional, and nothing in the almond/Spark stack needs it.
`git log -S'2.17.2' -- demo/` traces it to **`5d40c6f` "Add Demo for OSDS (#173)"**, the original demo commit, and the pre-PR notebook has the same two coordinates inline:
```
log4j:log4j-api:2.17.2
log4j:log4j-core:2.17.2
```
2.17.2 is the post-Log4Shell release from early 2022, i.e. simply what was current when the demo was first written.
So it now reads `log4j.version` from `versions.properties`, which `build_demo.sh` takes from the root pom — the demo tracks `<log4j.version>2.22.0</log4j.version>` and will not drift again on the next bump.
Since this is the one change here with actual runtime risk (log4j sits on the Spark classpath), I checked it in a running kernel rather than just building:
```
log4j on classpath: 2.22.0
java: 11.0.30-internal
spark: 3.4.2
xtable model: org.apache.xtable.model.InternalTable
```
with the predef compiling clean and printing `XTable demo kernel ready: …`.
--
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: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [727] Fix demo notebook to run against current main [incubator-xtable]
Posted by "rangareddy (via GitHub)" <gi...@apache.org>.
rangareddy commented on PR #850:
URL: https://github.com/apache/incubator-xtable/pull/850#issuecomment-5044331450
Yes @vinishjail97 Executed the demo.ipynb notebook end-to-end after debugging and resolving several issues encountered during the runs.
--
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: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [727] Fix demo notebook to run against current main [incubator-xtable]
Posted by "rangareddy (via GitHub)" <gi...@apache.org>.
rangareddy commented on PR #850:
URL: https://github.com/apache/incubator-xtable/pull/850#issuecomment-5188514842
@vinishjail97 all five comments are addressed and answered inline. Force-pushed as a **single commit** (`d6dd5fd`) — the 19 commits are squashed, and I verified the squash is byte-identical to the previous diff (same `git diff` checksum against the merge base) so nothing was lost or reordered in content.
The PR description needed a rewrite, not just an update: it documented a `demo/jupyter/setup-jdk11.sh` startup hook and inline notebook version pins, neither of which exists any more — JDK 11 and every dependency are baked into the image and all versions come from the root pom. It also named `xtable-core_2.12-0.2.0-SNAPSHOT.jar` explicitly, which the predef no longer does. There is a note at the top of the description flagging what changed, so anyone arriving mid-thread is not misled.
Two of your points found things worth calling out beyond the fix itself:
- **The `:latest` pin was hiding three assumptions, not one.** I added a second assert on `argv[0] == 'java'` alongside `assert paths`: if `argv[0]` were ever a wrapper script rather than the JVM, replacing index 0 and inserting `-Xmx4g` at index 1 would produce a broken kernel that the glob assert passes happily. Both fire correctly on negative tests. Also, `0.14.0-scala-2.12.17` does not exist — the scala-suffixed tags are single-kernel images, so the unsuffixed `0.14.1` is the right one for the `scala*` glob. It has the same manifest digest as `latest`, so the pin changes nothing today.
- **log4j 2.17.2 was carried over, not deliberate** — `git log -S'2.17.2'` traces it to `5d40c6f`, the original demo commit, and the pre-PR notebook has the same coordinates inline. Now tracks the root pom's 2.22.0, verified as 2.22.0 on a live kernel classpath.
I also fixed one thing you did not raise but which sits in the same area: `build_demo.sh` never cleared `demo/jars`, and `predef.sc` puts **every** jar there on the kernel classpath. After this branch picked up main's `0.5.0-SNAPSHOT` bump, my local `demo/jars` held both 0.2.0 and 0.5.0 copies of all three jars — two copies of the same classes, resolved in filesystem order. Added `rm -f demo/jars/xtable-*.jar` before the copies.
**On verification, to be straight with you:** I did not re-run the whole notebook this round. Cell 6 needs the Hive Metastore and I could not get one up locally — `apache/hive:4.0.0-alpha-2` is amd64-only and sat in emulated derby `initOrUpgradeSchema` without progressing on Apple Silicon. What I did verify is every mechanism these changes touch: the image builds with the pinned base and patches all three Scala kernels; the kernel starts, the predef compiles, and it reports log4j 2.22.0 / Java 11 / Spark 3.4.2 with `org.apache.xtable.model.InternalTable` resolving; `docker port` shows `127.0.0.1:8888` with the host's LAN address refused; and both asserts plus the missing-property check fail loudly when broken. The notebook cells themselves are untouched by this round. A confirming end-to-end run on x86 before merge would be welcome.
Three pre-existing things I noticed and deliberately did not change, each probably worth its own issue:
1. `almondsh/almond` is **amd64 only**, so the whole demo runs emulated on Apple Silicon.
2. Starting presto **appends `node.id=presto`** to the tracked `demo/presto/node.properties`, so running the demo dirties the working tree.
3. `container_name: hive-metastore` is a hardcoded global name, so the demo collides with any other local compose project using that name.
--
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: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org