sunchao commented on code in PR #5976:
URL: https://github.com/apache/datafusion-comet/pull/5976#discussion_r4039635679
##########
dev/ci/compute-changes.py:
##########
@@ -546,11 +571,16 @@ def event_allows(job, event):
def compute(files, event):
- """Return {job: bool}, folding the path filter and the event policy."""
- return {
+ """Return job flags, including main's warmer for shared native cache
inputs."""
+ selected = {
name: event_allows(name, event) and matches(patterns, files)
for name, patterns in FILTERS.items()
}
+ # Use the fingerprint's exact patterns and matcher for main's producer,
+ # including inputs owned by other workflows, without broadening PR jobs.
+ if event.get("name") == "push" and matches(NATIVE_LIBRARY_INPUTS, files):
Review Comment:
Done in 1dc4bf318. The push warmer now also calls
event_allows("build_linux", event). Added a regression case that removes push
from the policy and verifies a native input no longer selects the Linux job.
Removing the new gate makes that test fail.
##########
dev/ci/test-native-cache-key.py:
##########
@@ -0,0 +1,206 @@
+#!/usr/bin/env python3
+# 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.
+
+"""Check native cache boundaries and container checkout ownership with real
Git."""
+
+import importlib.util
+import io
+import os
+from pathlib import Path
+import subprocess
+import sys
+import tempfile
+import unittest
+from unittest.mock import patch
+
+
+SPEC = importlib.util.spec_from_file_location("native_cache_key",
Path(__file__).with_name("native-cache-key.py"))
+CACHE = importlib.util.module_from_spec(SPEC)
+SPEC.loader.exec_module(CACHE)
+
+
+class NativeCacheKeyTests(unittest.TestCase):
+ """Use disposable Git repositories and mock only installed tool
versions."""
+
+ def setUp(self):
+ """Create tracked native/JVM fixtures and JDK metadata; clean up after
each test."""
+ temporary = tempfile.TemporaryDirectory()
+ self.addCleanup(temporary.cleanup)
+ self.root = Path(temporary.name)
+ subprocess.run(["git", "init", "--quiet", str(self.root)], check=True)
+ self.inputs = {"native/Cargo.toml": "[workspace]\n",
"native/Cargo.lock": "version = 4\n",
+ "native/lib.rs": "fn native() {}\n",
"native/proto/expr.proto": "message Expr {}\n",
+ "spark/Plan.scala": "object Plan {}\n", "README.md":
"Comet\n",
+ ".github/workflows/README.md": "CI documentation\n",
+ ".github/workflows/pr_build_linux.yml": "jobs: {}\n",
+ ".github/workflows/spark_sql_test_reusable.yml": "jobs:
{}\n",
+ ".github/workflows/iceberg_spark_test_reusable.yml":
"jobs: {}\n",
+ ".github/workflows/spark_sql_writer_tests.yml": "jobs:
{}\n",
+ ".github/workflows/check_pr_title.yml": "jobs: {}\n",
+ ".github/actions/build-native-ci/action.yaml": "runs:
{}\n",
+ ".github/actions/setup-builder/action.yaml": "runs:
{}\n",
+ "dev/ci/compute-changes.py": "# shared native input
rules\n",
+ "contrib/delta/native/Cargo.toml": '[package]\nname =
"delta"\n',
+ "contrib/delta/native/src/lib.rs": "fn delta() {}\n",
+ "contrib/delta/native/Cargo.lock": "version = 4\n",
+ "contrib/a/b/native/Cargo.toml": '[package]\nname =
"nested"\n',
+ "native/core/benches/perf.rs": "fn benchmark() {}\n"}
+ for name, content in self.inputs.items():
+ self.write(name, content)
+ subprocess.run(["git", "add", "."], cwd=self.root, check=True)
+ self.write("jdk/release", 'JAVA_VERSION="17.0.1"\n')
+ self.env = {"JAVA_HOME": str(self.root / "jdk"), "CARGO_HOME":
str(self.root / "cargo"),
+ "RUSTFLAGS": "-Ctarget-cpu=x86-64-v3
-Clink-arg=-fuse-ld=bfd"}
+ self.versions = {"rustc": "rustc 1.90\nhost:
x86_64-unknown-linux-gnu\n",
+ "cargo": "cargo 1.90\n", "rustfmt": "rustfmt 1.8\n",
+ "dpkg-query": "libc6\t2.40\tamd64\n", "uname":
"x86_64\n"}
+
+ def write(self, name, content):
+ """Write fixture text under the temporary repository, creating its
parents."""
+ path = self.root / name
+ path.parent.mkdir(parents=True, exist_ok=True)
+ path.write_text(content)
+
+ def keys(self, profile="ci"):
+ """Return keys from real tracked files and deterministic tool version
responses."""
+ dependencies, sources = CACHE.source_inputs(self.root, profile)
+ with patch.object(CACHE, "command", side_effect=lambda args, cwd:
self.versions[args[0]]):
+ environment = CACHE.environment_inputs(self.root, self.env)
+ return CACHE.cache_keys(profile, dependencies, sources, environment)
+
+ def test_source_and_dependency_changes_invalidate_the_right_keys(self):
+ """Native/protobuf edits retain the dependency prefix; dependency
edits replace it."""
+ before = self.keys()
+ for name in ("native/lib.rs", "native/proto/expr.proto",
"native/Cargo.toml", "native/Cargo.lock",
+ "contrib/delta/native/Cargo.toml",
"dev/ci/compute-changes.py",
+ ".github/actions/build-native-ci/action.yaml",
".github/actions/setup-builder/action.yaml"):
+ with self.subTest(name=name):
+ self.write(name, self.inputs[name] + "changed\n")
+ after = self.keys()
+ self.assertNotEqual(before["source-key"], after["source-key"])
+ self.assertNotEqual(before["binary-key"], after["binary-key"])
+ if name.endswith(("Cargo.toml", "Cargo.lock")):
+ self.assertNotEqual(before["restore-prefix"],
after["restore-prefix"])
+ else:
+ self.assertEqual(before["restore-prefix"],
after["restore-prefix"])
+ self.write(name, self.inputs[name])
+
+ def test_generated_files_and_unrelated_jvm_edits_preserve_keys(self):
+ """Generated files and non-build edits preserve reuse; debug still
tracks benchmarks."""
+ before = self.keys()
+ debug = self.keys("debug")
+ for name in self.inputs:
+ if name.startswith(".github/workflows/"):
+ self.write(name, "unrelated test configuration\n")
+ self.env["GITHUB_RUN_ID"] = "12345"
+ self.assertEqual(before, self.keys())
+ self.assertEqual(debug, self.keys("debug"))
+ self.write("native/proto/src/generated/expr.rs", "generated Rust")
+ self.write("native/target/ci/libcomet.so", "compiled library")
+ self.write("spark/Plan.scala", "object NewPlan {}")
+ self.write("README.md", "updated docs")
+ self.write("contrib/delta/native/src/lib.rs", "fn changed_delta() {}")
+ self.write("contrib/delta/native/Cargo.lock", "version = 3\n")
+ self.write("contrib/a/b/native/Cargo.toml", '[package]\nname =
"changed_nested"\n')
+ self.write("native/core/benches/perf.rs", "fn changed_benchmark() {}")
+ self.assertEqual(before, self.keys())
+ self.assertNotEqual(debug["source-key"],
self.keys("debug")["source-key"])
+
+ def test_native_input_routing(self):
+ """Library inputs warm main; helper tests retain Linux coverage
without extra consumers."""
+ project = Path(__file__).resolve().parents[2]
+ route = CACHE.CHANGES.compute
+ _, sources = CACHE.source_inputs(project)
+ for name in [*sources, ".cargo/config.toml", "rust-toolchain",
"contrib/new/native/Cargo.toml"]:
+ self.assertTrue(route([name], {"name": "push"})["build_linux"],
name)
+ for name in ("contrib/delta/native/src/lib.rs",
"contrib/delta/native/Cargo.lock",
+ "contrib/a/b/native/Cargo.toml",
"contrib/a/b/native/x.rs"):
+ self.assertFalse(route([name], {"name": "push"})["build_linux"],
name)
+ self.assertFalse(route(["contrib/new/native/Cargo.toml"], {"name":
"pull_request"})["build_linux"])
+ for event in ("merge_group", "schedule"):
+ routed = route(["dev/ci/test-native-cache-key.py"], {"name":
event})
+ self.assertTrue(routed["build_linux" if event == "merge_group"
else "build_linux_all_profiles"])
+ self.assertFalse(any(selected for name, selected in routed.items()
+ if name.startswith(("spark_", "iceberg_"))))
+
+ def test_tools_jdk_flags_and_tracked_build_configuration_invalidate(self):
+ """Observed tool/package versions, Java metadata, flags and tracked
configs enter keys."""
+ before = self.keys()
+ for tool in self.versions:
+ with self.subTest(tool=tool):
+ old = self.versions[tool]
+ self.versions[tool] += "changed\n"
+ self.assertNotEqual(before["source-key"],
self.keys()["source-key"])
+ self.assertNotEqual(before["binary-key"],
self.keys()["binary-key"])
+ self.assertNotEqual(before["restore-prefix"],
self.keys()["restore-prefix"])
+ self.versions[tool] = old
+ self.write("jdk/release", 'JAVA_VERSION="17.0.2"\n')
+ self.assertNotEqual(before["binary-key"], self.keys()["binary-key"])
+ self.write("jdk/release", 'JAVA_VERSION="17.0.1"\n')
+ self.env["RUSTFLAGS"] += " -Copt-level=1"
+ self.assertNotEqual(before["binary-key"], self.keys()["binary-key"])
+ self.env["RUSTFLAGS"] = "-Ctarget-cpu=x86-64-v3
-Clink-arg=-fuse-ld=bfd"
+ for name in ("CC", "CXX", "CFLAGS", "LDFLAGS", "AR", "PROTOC",
"PROTOC_INCLUDE",
Review Comment:
Simplified in 1dc4bf318: the test now checks the captured environment
mapping directly, including exclusion of unrelated run variables, and uses one
representative override to verify invalidation of all three keys. I confirmed
the previous TARGET_CFLAGS case already failed when TARGET_ was removed; the
new assertion makes the selection contract more direct. I retained source-key
and binary-key checks for source changes because the two keys are constructed
independently, and a future change could omit sources from just one of them.
##########
dev/ci/test-native-cache-key.py:
##########
@@ -0,0 +1,206 @@
+#!/usr/bin/env python3
+# 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.
+
+"""Check native cache boundaries and container checkout ownership with real
Git."""
+
+import importlib.util
+import io
+import os
+from pathlib import Path
+import subprocess
+import sys
+import tempfile
+import unittest
+from unittest.mock import patch
+
+
+SPEC = importlib.util.spec_from_file_location("native_cache_key",
Path(__file__).with_name("native-cache-key.py"))
+CACHE = importlib.util.module_from_spec(SPEC)
+SPEC.loader.exec_module(CACHE)
+
+
+class NativeCacheKeyTests(unittest.TestCase):
+ """Use disposable Git repositories and mock only installed tool
versions."""
+
+ def setUp(self):
+ """Create tracked native/JVM fixtures and JDK metadata; clean up after
each test."""
+ temporary = tempfile.TemporaryDirectory()
+ self.addCleanup(temporary.cleanup)
+ self.root = Path(temporary.name)
+ subprocess.run(["git", "init", "--quiet", str(self.root)], check=True)
+ self.inputs = {"native/Cargo.toml": "[workspace]\n",
"native/Cargo.lock": "version = 4\n",
+ "native/lib.rs": "fn native() {}\n",
"native/proto/expr.proto": "message Expr {}\n",
+ "spark/Plan.scala": "object Plan {}\n", "README.md":
"Comet\n",
+ ".github/workflows/README.md": "CI documentation\n",
+ ".github/workflows/pr_build_linux.yml": "jobs: {}\n",
+ ".github/workflows/spark_sql_test_reusable.yml": "jobs:
{}\n",
+ ".github/workflows/iceberg_spark_test_reusable.yml":
"jobs: {}\n",
+ ".github/workflows/spark_sql_writer_tests.yml": "jobs:
{}\n",
+ ".github/workflows/check_pr_title.yml": "jobs: {}\n",
+ ".github/actions/build-native-ci/action.yaml": "runs:
{}\n",
+ ".github/actions/setup-builder/action.yaml": "runs:
{}\n",
+ "dev/ci/compute-changes.py": "# shared native input
rules\n",
+ "contrib/delta/native/Cargo.toml": '[package]\nname =
"delta"\n',
+ "contrib/delta/native/src/lib.rs": "fn delta() {}\n",
+ "contrib/delta/native/Cargo.lock": "version = 4\n",
+ "contrib/a/b/native/Cargo.toml": '[package]\nname =
"nested"\n',
+ "native/core/benches/perf.rs": "fn benchmark() {}\n"}
+ for name, content in self.inputs.items():
+ self.write(name, content)
+ subprocess.run(["git", "add", "."], cwd=self.root, check=True)
+ self.write("jdk/release", 'JAVA_VERSION="17.0.1"\n')
+ self.env = {"JAVA_HOME": str(self.root / "jdk"), "CARGO_HOME":
str(self.root / "cargo"),
+ "RUSTFLAGS": "-Ctarget-cpu=x86-64-v3
-Clink-arg=-fuse-ld=bfd"}
+ self.versions = {"rustc": "rustc 1.90\nhost:
x86_64-unknown-linux-gnu\n",
+ "cargo": "cargo 1.90\n", "rustfmt": "rustfmt 1.8\n",
+ "dpkg-query": "libc6\t2.40\tamd64\n", "uname":
"x86_64\n"}
+
+ def write(self, name, content):
+ """Write fixture text under the temporary repository, creating its
parents."""
+ path = self.root / name
+ path.parent.mkdir(parents=True, exist_ok=True)
+ path.write_text(content)
+
+ def keys(self, profile="ci"):
+ """Return keys from real tracked files and deterministic tool version
responses."""
+ dependencies, sources = CACHE.source_inputs(self.root, profile)
+ with patch.object(CACHE, "command", side_effect=lambda args, cwd:
self.versions[args[0]]):
+ environment = CACHE.environment_inputs(self.root, self.env)
+ return CACHE.cache_keys(profile, dependencies, sources, environment)
+
+ def test_source_and_dependency_changes_invalidate_the_right_keys(self):
+ """Native/protobuf edits retain the dependency prefix; dependency
edits replace it."""
+ before = self.keys()
+ for name in ("native/lib.rs", "native/proto/expr.proto",
"native/Cargo.toml", "native/Cargo.lock",
+ "contrib/delta/native/Cargo.toml",
"dev/ci/compute-changes.py",
+ ".github/actions/build-native-ci/action.yaml",
".github/actions/setup-builder/action.yaml"):
+ with self.subTest(name=name):
+ self.write(name, self.inputs[name] + "changed\n")
+ after = self.keys()
+ self.assertNotEqual(before["source-key"], after["source-key"])
+ self.assertNotEqual(before["binary-key"], after["binary-key"])
+ if name.endswith(("Cargo.toml", "Cargo.lock")):
+ self.assertNotEqual(before["restore-prefix"],
after["restore-prefix"])
+ else:
+ self.assertEqual(before["restore-prefix"],
after["restore-prefix"])
+ self.write(name, self.inputs[name])
+
+ def test_generated_files_and_unrelated_jvm_edits_preserve_keys(self):
+ """Generated files and non-build edits preserve reuse; debug still
tracks benchmarks."""
+ before = self.keys()
+ debug = self.keys("debug")
+ for name in self.inputs:
+ if name.startswith(".github/workflows/"):
+ self.write(name, "unrelated test configuration\n")
+ self.env["GITHUB_RUN_ID"] = "12345"
+ self.assertEqual(before, self.keys())
+ self.assertEqual(debug, self.keys("debug"))
+ self.write("native/proto/src/generated/expr.rs", "generated Rust")
+ self.write("native/target/ci/libcomet.so", "compiled library")
+ self.write("spark/Plan.scala", "object NewPlan {}")
+ self.write("README.md", "updated docs")
+ self.write("contrib/delta/native/src/lib.rs", "fn changed_delta() {}")
+ self.write("contrib/delta/native/Cargo.lock", "version = 3\n")
+ self.write("contrib/a/b/native/Cargo.toml", '[package]\nname =
"changed_nested"\n')
+ self.write("native/core/benches/perf.rs", "fn changed_benchmark() {}")
+ self.assertEqual(before, self.keys())
+ self.assertNotEqual(debug["source-key"],
self.keys("debug")["source-key"])
+
+ def test_native_input_routing(self):
+ """Library inputs warm main; helper tests retain Linux coverage
without extra consumers."""
+ project = Path(__file__).resolve().parents[2]
+ route = CACHE.CHANGES.compute
+ _, sources = CACHE.source_inputs(project)
Review Comment:
Done in 1dc4bf318. Replaced the repository inventory scan with explicit
representative source, protobuf, contrib-manifest, Cargo-config, action/helper
and toolchain paths, plus excluded documentation, benchmarks and disabled
contrib sources. It also covers the policy gate and test-only routing. No
generated glob-fixture machinery was added.
##########
.github/workflows/README.md:
##########
@@ -404,6 +404,64 @@ entry through `restore-keys` and downloads whatever else
it needs, which is
what a cold pull request already did. See the push-tier discussion above for
which jobs do run on main and therefore do write.
+## Reusing Linux native builds
+
+The Linux, Spark SQL, Iceberg and manual writer workflows call
+`.github/actions/build-native-ci` after checkout and `setup-builder`. An exact
+cache hit restores `native/target/ci/libcomet.so` and skips Cargo. A miss
restores
+an incremental cache and runs `cargo build --locked --profile ci`. Artifacts
and
+downstream tests use the same paths in either case.
+`--locked` deliberately fails when a manifest change requires updating
+`native/Cargo.lock`; contributors must commit that lockfile update with the
change.
+
+`dev/ci/native-cache-key.py` snapshots tracked native/protobuf/dependency
files,
+Cargo configuration and the native build recipes before Cargo generates source
+files. The key also includes Rust versions, installed system package
+versions, architecture, JDK release/path and the build environment: Cargo/Rust
+settings, C/C++ compiler and flag overrides (including target-specific
variants),
+and the HDFS library overrides used by the default dependencies. The helper
targets
+our official Rust container and `setup-builder`. Adding external tools or files
+requires updating this contract; recording an override's path does not identify
+arbitrary contents stored there.
+
+The shared build and setup actions are fingerprinted; the four caller workflows
+are not. Their selected Rust/JDK versions and build environment are observed
+directly, so editing a test matrix or shard does not force a native rebuild.
+Spark-only edits, documentation and generated files also preserve the key;
+native/protobuf changes invalidate it. Optional contrib crates contribute
+their manifests, which Cargo resolves even with their features disabled, but
not
+their Rust sources or standalone lockfiles. Benchmarks enter the debug cache
key
+but not the library key. The input lists and glob matcher are shared with
main's
+cache routing in `compute-changes.py`. The shared action uses portable
`x86-64-v3`
+code generation.
+
+The incremental cache contains the effective `CARGO_HOME` registry/git
directories
+and `native/target`. In the Rust container, correcting `~/.cargo` to
+`/usr/local/cargo` adds the registry and Git checkouts that the old entry did
not
+contain. The incremental entry therefore grows alongside the addition of the
+separate finished-library entry.
+Its dependency prefix permits reuse after source changes
+within the same build environment, but every restore still invokes Cargo.
+Environment changes also invalidate this fallback: native dependencies compile
C
+against JNI headers and cache build-script outputs that Cargo does not fully
+invalidate after external compiler or JDK changes. This can miss after
unrelated
+package updates, but prevents reusing those objects under a new library key.
+The Rust test job uses a separate debug key and continues to run all checks
and tests.
+
+Only pushes to `main` save either cache. Main always compiles to keep the
+incremental cache warm. Other runs consume matching entries; a cold or evicted
+cache builds normally. Changes to shared native inputs owned by other workflows
+also trigger main's cache warmer. GitHub Actions handles cache storage and
+restoration.
+
+Preflight tests key invalidation, generated-file stability, container checkout
Review Comment:
Shortened the native-cache README section and moved the initial
measurement/verification instructions into the PR description. The README now
describes the durable contract: inputs, invalidation, writers/consumers,
exact-hit behavior, --locked, supported builder assumptions and the resulting
cache-budget constraint. It also reflects the new main double-hit optimization.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
