sunchao commented on code in PR #4952: URL: https://github.com/apache/datafusion-comet/pull/4952#discussion_r3807494654
########## dev/verify-contrib-delta-gate.sh: ########## @@ -0,0 +1,288 @@ +#!/usr/bin/env 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. +# +# Verify the `contrib-delta` build gate keeps Delta surface out of default builds. +# +# Three independent layers are checked: +# 1. Cargo: default `cargo build` doesn't compile `comet-contrib-delta` and +# doesn't pull `delta_kernel` into the dependency tree. +# 2. Maven: default `mvn ... package` doesn't compile any +# `org/apache/comet/contrib/` classes and doesn't pull `io.delta:*` deps. +# 3. Symbol/size: the resulting `libcomet` (`.so` on Linux, `.dylib` on macOS) from the default build is +# meaningfully smaller than the contrib-enabled build, and carries no +# `comet_contrib_delta`/`delta_kernel`/etc. external symbols. +# +# Exit non-zero on the first failure. Designed to be wired into CI so a future +# change that leaks Delta into core gets caught immediately. + +set -euo pipefail + +ROOT="$(cd "$(dirname "$0")/.." && pwd)" +NATIVE_DIR="$ROOT/native" +SPARK_DIR="$ROOT/spark" + +# CI containers check out the repo as a different user than the job runs as, so git refuses to +# operate on it ("detected dubious ownership") -- which can make Maven plugins that read git +# metadata fail fast with no useful output. Mark the tree safe up front (best-effort). +git config --global --add safe.directory "$ROOT" 2>/dev/null || true +git config --global --add safe.directory '*' 2>/dev/null || true Review Comment: [P2] Keep the Git ownership exception scoped to this checkout Please remove the global wildcard. Running this developer-facing script appends `safe.directory=*` to the caller's persistent Git configuration before the build starts, so the setting remains even if a later check fails. It disables Git's ownership safeguard for unrelated repositories as well. The preceding line already trusts the intended checkout; any additional CI exception should be scoped to that checkout or process. See the [Git documentation](https://git-scm.com/docs/git-config#Documentation/git-config.txt-safedirectory). ########## spark/pom.xml: ########## @@ -339,6 +371,69 @@ under the License. </dependency> </dependencies> </profile> + <!-- + Delta Lake integration. Activates the contrib/delta/ subtree: + compiles contrib/delta/src/main/scala/ into comet-spark.jar + pulls in io.delta:delta-spark at compile/test scope so the contrib's + reflective helpers and tests have the types they need + Default builds (no -P) carry zero Delta surface. Pair with the matching + 'features contrib-delta' Cargo flag when building the native library. + --> + <profile> + <id>contrib-delta</id> Review Comment: [P2] Preserve the implicit Spark profile when enabling Delta Activating `contrib-delta` disables this POM's `activeByDefault` `spark-4.1` profile. I compared the effective POMs: `-Pcontrib-delta` still targets Spark 4.1 and adds Delta, but drops the Iceberg runtime and Jetty test dependencies; `-Pspark-4.1,contrib-delta` restores them. Existing test sources such as `CometEqualityDeletes.scala` directly import Iceberg types, so the advertised `./mvnw package -Pcontrib-delta -DskipTests` invocation lacks classes needed during test compilation. Please preserve or explicitly require the matching Spark profile and cover the implicit-default invocation in the gate. This was verified through the Maven model, not a full compile. ########## contrib/delta/native/Cargo.lock: ########## @@ -0,0 +1,2765 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 4 + +[[package]] +name = "adler2" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "320119579fcad9c21884f5c4861d16174d0e06250625266f50fe6898340abefa" + +[[package]] +name = "ahash" +version = "0.8.12" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "5a15f179cd60c4584b8a8c596927aadc462e27f2ca70c04e0071964a73ba7a75" +dependencies = [ + "cfg-if", + "const-random", + "getrandom 0.3.4", + "once_cell", + "version_check", + "zerocopy", +] + +[[package]] +name = "aho-corasick" +version = "1.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "ddd31a130427c27518df266943a5308ed92d4b226cc639f5a8f1002816174301" +dependencies = [ + "memchr", +] + +[[package]] +name = "alloc-no-stdlib" +version = "2.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "cc7bb162ec39d46ab1ca8c77bf72e890535becd1751bb45f64c597edb4c8c6b3" + +[[package]] +name = "alloc-stdlib" +version = "0.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "0e76a019e91224d279006ff972f1e984179a6e9feb050adba6ce8274aef23195" +dependencies = [ + "alloc-no-stdlib", +] + +[[package]] +name = "allocator-api2" +version = "0.2.21" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "683d7910e743518b0e34f1186f92494becacb047c7b6bf616c96772180fef923" + +[[package]] +name = "android_system_properties" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "819e7219dbd41043ac279b19830f2efc897156490d7fd6ea916720117ee66311" +dependencies = [ + "libc", +] + +[[package]] +name = "anyhow" +version = "1.0.102" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "7f202df86484c868dbad7eaa557ef785d5c66295e41b460ef922eca0723b842c" + +[[package]] +name = "arrow" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "378530e55cd479eda3c14eb345310799717e6f76d0c332041e8487022166b471" +dependencies = [ + "arrow-arith", + "arrow-array", + "arrow-buffer", + "arrow-cast", + "arrow-csv", + "arrow-data", + "arrow-ipc", + "arrow-json", + "arrow-ord", + "arrow-row", + "arrow-schema", + "arrow-select", + "arrow-string", +] + +[[package]] +name = "arrow-arith" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "a0ab212d2c1886e802f51c5212d78ebbcbb0bec980fff9dadc1eb8d45cd0b738" +dependencies = [ + "arrow-array", + "arrow-buffer", + "arrow-data", + "arrow-schema", + "chrono", + "num-traits", +] + +[[package]] +name = "arrow-array" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "cfd33d3e92f207444098c75b42de99d329562be0cf686b307b097cc52b4e999e" +dependencies = [ + "ahash", + "arrow-buffer", + "arrow-data", + "arrow-schema", + "chrono", + "chrono-tz", + "half", + "hashbrown 0.17.1", + "num-complex", + "num-integer", + "num-traits", +] + +[[package]] +name = "arrow-buffer" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "0c6cd424c2693bcdbc150d843dc9d4d137dd2de4782ce6df491ad11a3a0416c0" +dependencies = [ + "bytes", + "half", + "num-bigint", + "num-traits", +] + +[[package]] +name = "arrow-cast" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "4c5aefb56a2c02e9e2b30746241058b85f8983f0fcff2ba0c6d09006e1cded7f" +dependencies = [ + "arrow-array", + "arrow-buffer", + "arrow-data", + "arrow-ord", + "arrow-schema", + "arrow-select", + "atoi", + "base64", + "chrono", + "comfy-table", + "half", + "lexical-core", + "num-traits", + "ryu", +] + +[[package]] +name = "arrow-csv" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "e94e8cf7e517657a52b91ea1263acf38c4ca62a84655d72458a3359b12ab97de" +dependencies = [ + "arrow-array", + "arrow-cast", + "arrow-schema", + "chrono", + "csv", + "csv-core", + "regex", +] + +[[package]] +name = "arrow-data" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "3c88210023a2bfee1896af366309a3028fc3bcbd6515fa29a7990ee1baa08ee0" +dependencies = [ + "arrow-buffer", + "arrow-schema", + "half", + "num-integer", + "num-traits", +] + +[[package]] +name = "arrow-ipc" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "238438f0834483703d88896db6fe5a7138b2230debc31b34c0336c2996e3c64f" +dependencies = [ + "arrow-array", + "arrow-buffer", + "arrow-data", + "arrow-schema", + "arrow-select", + "flatbuffers", + "lz4_flex", + "zstd", +] + +[[package]] +name = "arrow-json" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "205ca2119e6d679d5c133c6f30e68f027738d95ed948cf77677ea69c7800036b" +dependencies = [ + "arrow-array", + "arrow-buffer", + "arrow-cast", + "arrow-ord", + "arrow-schema", + "arrow-select", + "chrono", + "half", + "indexmap", + "itoa", + "lexical-core", + "memchr", + "num-traits", + "ryu", + "serde_core", + "serde_json", + "simdutf8", +] + +[[package]] +name = "arrow-ord" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "1bffd8fd2579286a5d63bac898159873e5094a79009940bcb42bbfce4f19f1d0" +dependencies = [ + "arrow-array", + "arrow-buffer", + "arrow-data", + "arrow-schema", + "arrow-select", +] + +[[package]] +name = "arrow-row" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "bab5994731204603c73ba69267616c50f80780774c6bb0476f1f830625115e0c" +dependencies = [ + "arrow-array", + "arrow-buffer", + "arrow-data", + "arrow-schema", + "half", +] + +[[package]] +name = "arrow-schema" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "f633dbfdf39c039ada1bf9e34c694816eb71fbb7dc78f613993b7245e078a1ed" +dependencies = [ + "serde_core", + "serde_json", +] + +[[package]] +name = "arrow-select" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "8cd065c54172ac787cf3f2f8d4107e0d3fdc26edba76fdf4f4cc170258942222" +dependencies = [ + "ahash", + "arrow-array", + "arrow-buffer", + "arrow-data", + "arrow-schema", + "num-traits", +] + +[[package]] +name = "arrow-string" +version = "58.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "29dd7cda3ab9692f43a2e4acc444d760cc17b12bb6d8232ddf64e9bab7c06b42" +dependencies = [ + "arrow-array", + "arrow-buffer", + "arrow-data", + "arrow-schema", + "arrow-select", + "memchr", + "num-traits", + "regex", + "regex-syntax", +] + +[[package]] +name = "async-trait" +version = "0.1.89" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "9035ad2d096bed7955a320ee7e2230574d28fd3c3a0f186cbea1ff3c7eed5dbb" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "atoi" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "f28d99ec8bfea296261ca1af174f24225171fea9664ba9003cbebee704810528" +dependencies = [ + "num-traits", +] + +[[package]] +name = "autocfg" +version = "1.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "f2032f911046de80f0a198e0901378627c33f59ea0ac00e363d481118bd70a53" + +[[package]] +name = "base64" +version = "0.22.1" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "72b3254f16251a8381aa12e40e3c4d2f0199f8c6508fbecb9d91f575e0fbb8c6" + +[[package]] +name = "bitflags" +version = "2.13.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "b4388bee8683e3d04af747c73422af53102d2bd24d9eadb6cbc100baef4b43f8" + +[[package]] +name = "brotli" +version = "8.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "5cc91aac060a7a1e25823bdccbfb6af1875b88f17c6daac97894eed8207166b3" +dependencies = [ + "alloc-no-stdlib", + "alloc-stdlib", + "brotli-decompressor", +] + +[[package]] +name = "brotli-decompressor" +version = "5.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "3a32acac15fe1967bc3986b2a6347dffc965602354ea6f450ad07e8bfd253583" +dependencies = [ + "alloc-no-stdlib", + "alloc-stdlib", +] + +[[package]] +name = "bumpalo" +version = "3.20.3" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "72f5acc6cb2ba439de613abc23857ec3d78374d8ed5ac84e9d11336e87da8649" + +[[package]] +name = "byteorder" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "1fd0f2584146f6f2ef48085050886acf353beff7305ebd1ae69500e27c67f64b" + +[[package]] +name = "bytes" +version = "1.12.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "8ae3f5d315924270530207e2a68396c3cc547f6dca3fbdca317cfb1a51edb593" + +[[package]] +name = "cc" +version = "1.2.65" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "e228eec9be7c17ccb640b59b36a5cd805ea2a564a4c5e162c2f659fea30d3b96" +dependencies = [ + "find-msvc-tools", + "jobserver", + "libc", + "shlex", +] + +[[package]] +name = "cfg-if" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" + +[[package]] +name = "chrono" +version = "0.4.45" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "1aa79e62e7697b8e29b513a68abacf485adcd1fe8284a4316c5ae868e6633327" +dependencies = [ + "iana-time-zone", + "num-traits", + "windows-link", +] + +[[package]] +name = "chrono-tz" +version = "0.10.4" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "a6139a8597ed92cf816dfb33f5dd6cf0bb93a6adc938f11039f371bc5bcd26c3" +dependencies = [ + "chrono", + "phf", +] + +[[package]] +name = "comet-contrib-delta" +version = "0.18.0" +dependencies = [ + "datafusion", + "datafusion-comet-proto", +] + +[[package]] +name = "comfy-table" +version = "7.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "958c5d6ecf1f214b4c2bbbbf6ab9523a864bd136dcf71a7e8904799acfe1ad47" +dependencies = [ + "unicode-segmentation", + "unicode-width", +] + +[[package]] +name = "const-random" +version = "0.1.18" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "87e00182fe74b066627d63b85fd550ac2998d4b0bd86bfed477a0ae4c7c71359" +dependencies = [ + "const-random-macro", +] + +[[package]] +name = "const-random-macro" +version = "0.1.16" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "f9d839f2a20b0aee515dc581a6172f2321f96cab76c1a38a4c584a194955390e" +dependencies = [ + "getrandom 0.2.17", + "once_cell", + "tiny-keccak", +] + +[[package]] +name = "core-foundation-sys" +version = "0.8.7" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" + +[[package]] +name = "crossbeam-utils" +version = "0.8.21" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "d0a5c400df2834b80a4c3327b3aad3a4c4cd4de0629063962b03235697506a28" + +[[package]] +name = "crunchy" +version = "0.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "460fbee9c2c2f33933d720630a6a0bac33ba7053db5344fac858d4b8952d77d5" + +[[package]] +name = "csv" +version = "1.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "52cd9d68cf7efc6ddfaaee42e7288d3a99d613d4b50f76ce9827ae0c6e14f938" +dependencies = [ + "csv-core", + "itoa", + "ryu", + "serde_core", +] + +[[package]] +name = "csv-core" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "704a3c26996a80471189265814dbc2c257598b96b8a7feae2d31ace646bb9782" +dependencies = [ + "memchr", +] + +[[package]] +name = "dashmap" +version = "6.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "e6361d5c062261c78a176addb82d4c821ae42bed6089de0e12603cd25de2059c" +dependencies = [ + "cfg-if", + "crossbeam-utils", + "hashbrown 0.14.5", + "lock_api", + "once_cell", + "parking_lot_core", +] + +[[package]] +name = "datafusion" +version = "54.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "997a31e15872606a49478e670c58302094c97cb96abb0a7d60720f8e92170040" +dependencies = [ + "arrow", + "arrow-schema", + "async-trait", + "chrono", + "datafusion-catalog", + "datafusion-catalog-listing", + "datafusion-common", + "datafusion-common-runtime", + "datafusion-datasource", + "datafusion-datasource-arrow", + "datafusion-datasource-csv", + "datafusion-datasource-json", + "datafusion-datasource-parquet", + "datafusion-execution", + "datafusion-expr", + "datafusion-expr-common", + "datafusion-functions", + "datafusion-functions-aggregate", + "datafusion-functions-table", + "datafusion-functions-window", + "datafusion-optimizer", + "datafusion-physical-expr", + "datafusion-physical-expr-adapter", + "datafusion-physical-expr-common", + "datafusion-physical-optimizer", + "datafusion-physical-plan", + "datafusion-session", + "futures", + "indexmap", + "itertools", + "log", + "object_store", + "parking_lot", + "parquet", + "tempfile", + "tokio", + "url", + "uuid", +] + +[[package]] +name = "datafusion-catalog" +version = "54.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "f7dd61161508f8f5fa1107774ea687bd753c22d83a32eebf963549f89de14139" +dependencies = [ + "arrow", + "async-trait", + "dashmap", + "datafusion-common", + "datafusion-common-runtime", + "datafusion-datasource", + "datafusion-execution", + "datafusion-expr", + "datafusion-physical-expr", + "datafusion-physical-plan", + "datafusion-session", + "futures", + "itertools", + "log", + "object_store", + "parking_lot", + "tokio", +] + +[[package]] +name = "datafusion-catalog-listing" +version = "54.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index"; +checksum = "897c70f871277f9ce99aa38347be0d679bbe3e617156c4d2a8378cec8a2a0891" +dependencies = [ + "arrow", + "async-trait", + "datafusion-catalog", + "datafusion-common", + "datafusion-datasource", + "datafusion-execution", + "datafusion-expr", + "datafusion-physical-expr", + "datafusion-physical-expr-adapter", + "datafusion-physical-expr-common", + "datafusion-physical-plan", + "futures", + "itertools", + "log", + "object_store", +] + +[[package]] +name = "datafusion-comet-proto" +version = "1.0.0" Review Comment: [P3] Refresh the standalone contrib lockfile This entry still records the path dependency `datafusion-comet-proto` as `1.0.0`, while `native/proto` now inherits workspace version `1.1.0`. `cargo tree --manifest-path contrib/delta/native/Cargo.toml --locked --offline --prefix none --depth 1` exits 101 because Cargo needs to update this lockfile. Please regenerate it so standalone locked crate commands work. The core build uses the separate, valid `native/Cargo.lock`, so this is not an enabled-core build failure. ########## spark/src/main/scala/org/apache/comet/rules/CometScanContrib.scala: ########## @@ -0,0 +1,210 @@ +/* + * 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. + */ + +package org.apache.comet.rules + +import java.util.ServiceLoader + +import scala.jdk.CollectionConverters._ +import scala.util.control.NonFatal + +import org.apache.spark.internal.Logging +import org.apache.spark.sql.SparkSession +import org.apache.spark.sql.execution.{FileSourceScanExec, SparkPlan} +import org.apache.spark.sql.execution.datasources.HadoopFsRelation +import org.apache.spark.sql.execution.datasources.v2.BatchScanExec + +import org.apache.comet.CometConf +import org.apache.comet.serde.CometOperatorSerde + +/** + * Format-agnostic hook that lets an optional, out-of-tree contrib (Delta, Lance, ...) claim a + * scan before Comet's built-in scan handling runs. This is the scan-rule counterpart to + * [[org.apache.spark.sql.comet.PlanDataInjector]]: core holds no compile-time reference to any + * contrib and names none of them -- implementations are discovered at runtime via the JDK + * [[ServiceLoader]], so default builds (which ship no service file) see an empty registry and + * zero contrib surface. + * + * A contrib ships: + * - an implementation of this trait (e.g. `contrib/delta/.../DeltaScanRuleContrib`), and + * - a `META-INF/services/org.apache.comet.rules.CometScanContrib` resource naming it. + * + * Both hooks default to `None` so a contrib overrides only the scan kind(s) it handles. + * + * ==Ownership contract== + * + * An implementation '''MUST''' return `None` for any scan it does not own. Registered contribs + * are offered a scan one at a time and the '''first''' claim wins, so a contrib that claims a + * scan belonging to another format does not merely mis-handle that scan -- it hides it from the + * contrib that could have read it, and the outcome depends on ServiceLoader ordering, which is + * not specified. Decide ownership from something definitive about the scan itself (the relation's + * `fileFormat` class, the table's provider, a catalog type) rather than from a heuristic such as + * a path or table-name pattern, which another format may also match. + * + * "Own but cannot handle" is a distinct case from "not mine", and both are expressible: to + * decline a scan it owns, a contrib returns `Some(withFallbackReason(scanExec, "..."))` -- + * claiming the scan and terminating it with a diagnosable reason -- rather than returning `None` + * and letting Comet's built-in handling attempt a format it does not understand. + */ +trait CometScanContrib { + + /** + * V1 (`FileSourceScanExec`) hook. Return `Some(plan)` to claim the scan -- either a transformed + * plan node or an explicit fallback the contrib produced via `withFallbackReason`. Return + * `None` to pass, letting Comet's generic V1 handling proceed; see the ownership contract + * above. A claiming contrib is responsible for its own metadata-column handling (the generic + * guard in [[CometScanRule]] runs only on the pass path). + */ + def tryTransformV1( + plan: SparkPlan, + session: SparkSession, + scanExec: FileSourceScanExec, + relation: HadoopFsRelation): Option[SparkPlan] = None + + /** + * V2 (`BatchScanExec`) hook. Return `Some(plan)` to claim the scan, `None` to pass; see the + * ownership contract above. + */ + def tryTransformV2(scanExec: BatchScanExec): Option[SparkPlan] = None +} + +object CometScanContrib extends Logging { + + // Discovered contrib scan handlers. Mirrors `PlanDataInjector.injectors`: the standard JDK + // ServiceLoader forces the provider iterator here so a misbuilt contrib jar (malformed service + // file, or a listed provider that can't be instantiated) surfaces as a warning rather than + // taking down the planner. Default builds carry no service file, so this is empty and there is + // no contrib surface at runtime. There are no built-in (in-core) contribs today; the format + // built-ins (Parquet/Iceberg) keep their existing paths in CometScanRule. + private[comet] lazy val contribs: Seq[CometScanContrib] = + try { + ServiceLoader.load(classOf[CometScanContrib], getClass.getClassLoader).asScala.toSeq Review Comment: [P2] Discover separate contrib jars through Spark's context classloader When Comet is installed on `spark.driver.extraClassPath` and a separate contrib is supplied through `--jars`, Comet's defining loader is the parent of Spark's user-jar loader. Passing that parent explicitly means the child-only service is invisible, so the registry silently ignores the contrib. An isolated test against this unchanged class returns `None` through the production registry while the thread-context loader discovers and dispatches the same provider. Please use Spark's context classloader with an appropriate fallback, as the existing `org.apache.comet.util.ClassLoaders` helper does, and exercise the production registry in the regression test. Contrib classes compiled into the same core jar are unaffected. ########## spark/src/main/scala/org/apache/comet/rules/CometScanContrib.scala: ########## @@ -0,0 +1,210 @@ +/* + * 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. + */ + +package org.apache.comet.rules + +import java.util.ServiceLoader + +import scala.jdk.CollectionConverters._ +import scala.util.control.NonFatal + +import org.apache.spark.internal.Logging +import org.apache.spark.sql.SparkSession +import org.apache.spark.sql.execution.{FileSourceScanExec, SparkPlan} +import org.apache.spark.sql.execution.datasources.HadoopFsRelation +import org.apache.spark.sql.execution.datasources.v2.BatchScanExec + +import org.apache.comet.CometConf +import org.apache.comet.serde.CometOperatorSerde + +/** + * Format-agnostic hook that lets an optional, out-of-tree contrib (Delta, Lance, ...) claim a + * scan before Comet's built-in scan handling runs. This is the scan-rule counterpart to + * [[org.apache.spark.sql.comet.PlanDataInjector]]: core holds no compile-time reference to any + * contrib and names none of them -- implementations are discovered at runtime via the JDK + * [[ServiceLoader]], so default builds (which ship no service file) see an empty registry and + * zero contrib surface. + * + * A contrib ships: + * - an implementation of this trait (e.g. `contrib/delta/.../DeltaScanRuleContrib`), and + * - a `META-INF/services/org.apache.comet.rules.CometScanContrib` resource naming it. + * + * Both hooks default to `None` so a contrib overrides only the scan kind(s) it handles. + * + * ==Ownership contract== + * + * An implementation '''MUST''' return `None` for any scan it does not own. Registered contribs + * are offered a scan one at a time and the '''first''' claim wins, so a contrib that claims a + * scan belonging to another format does not merely mis-handle that scan -- it hides it from the + * contrib that could have read it, and the outcome depends on ServiceLoader ordering, which is + * not specified. Decide ownership from something definitive about the scan itself (the relation's + * `fileFormat` class, the table's provider, a catalog type) rather than from a heuristic such as + * a path or table-name pattern, which another format may also match. + * + * "Own but cannot handle" is a distinct case from "not mine", and both are expressible: to + * decline a scan it owns, a contrib returns `Some(withFallbackReason(scanExec, "..."))` -- + * claiming the scan and terminating it with a diagnosable reason -- rather than returning `None` + * and letting Comet's built-in handling attempt a format it does not understand. + */ +trait CometScanContrib { + + /** + * V1 (`FileSourceScanExec`) hook. Return `Some(plan)` to claim the scan -- either a transformed + * plan node or an explicit fallback the contrib produced via `withFallbackReason`. Return + * `None` to pass, letting Comet's generic V1 handling proceed; see the ownership contract + * above. A claiming contrib is responsible for its own metadata-column handling (the generic + * guard in [[CometScanRule]] runs only on the pass path). + */ + def tryTransformV1( + plan: SparkPlan, + session: SparkSession, + scanExec: FileSourceScanExec, + relation: HadoopFsRelation): Option[SparkPlan] = None + + /** + * V2 (`BatchScanExec`) hook. Return `Some(plan)` to claim the scan, `None` to pass; see the + * ownership contract above. + */ + def tryTransformV2(scanExec: BatchScanExec): Option[SparkPlan] = None +} + +object CometScanContrib extends Logging { + + // Discovered contrib scan handlers. Mirrors `PlanDataInjector.injectors`: the standard JDK + // ServiceLoader forces the provider iterator here so a misbuilt contrib jar (malformed service + // file, or a listed provider that can't be instantiated) surfaces as a warning rather than + // taking down the planner. Default builds carry no service file, so this is empty and there is + // no contrib surface at runtime. There are no built-in (in-core) contribs today; the format + // built-ins (Parquet/Iceberg) keep their existing paths in CometScanRule. + private[comet] lazy val contribs: Seq[CometScanContrib] = + try { + ServiceLoader.load(classOf[CometScanContrib], getClass.getClassLoader).asScala.toSeq Review Comment: [P2] Materialize service discovery inside the exception handler On supported Scala 2.12, `.asScala.toSeq` produces a lazy `Stream`. With a valid declining provider followed by a missing or uninstantiable provider, this `try` returns before the bad entry is visited. `firstClaimFrom` later forces it outside the discovery/per-hook catches, so `ServiceConfigurationError` fails an otherwise runnable scan. I reproduced this with the unchanged class on Scala 2.12.18; replacing `toSeq` with `toList` makes the existing catch work. Please use a strict collection here and in the new `CometConfigProvider` registry, and test a malformed later service entry. Conflict-detection mode can expose the same failure even when the first provider claims the scan. -- 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]
