sunchao commented on code in PR #4952: URL: https://github.com/apache/datafusion-comet/pull/4952#discussion_r3809912668
########## dev/verify-contrib-delta-gate.sh: ########## @@ -0,0 +1,300 @@ +#!/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. +# +# Grant the exception through the environment rather than `git config --global`, so it lasts +# exactly as long as this process and its children. Writing it to the global config would outlive +# the script -- including when a check below exits non-zero -- leaving the setting in the +# developer's ~/.gitconfig; `--add` also appended a duplicate entry on every run. The previous +# `safe.directory=*` was worse still: it disabled git's ownership safeguard for every repository +# on the machine, not just this checkout. +# +# GIT_CONFIG_COUNT/KEY/VALUE needs git >= 2.31; CI images are well past that. Anything older +# simply does not get the exception, which is the pre-existing behaviour for a normally-owned +# checkout anyway. +export GIT_CONFIG_COUNT=1 +export GIT_CONFIG_KEY_0=safe.directory +export GIT_CONFIG_VALUE_0="$ROOT" Review Comment: [P2] Append the checkout exception to inherited Git configuration The persistent global-config issue is fixed, but setting `GIT_CONFIG_COUNT=1` and replacing entry zero discards any runtime Git configuration inherited from the caller. An isolated probe confirmed that an existing `http.proxy` and `protocol.file.allow=never` both disappear after these exports. This can also drop credentials or URL rewrites needed by child Git commands; the gate's Cargo dependency tree includes the Iceberg Git dependencies, so it matters when Cargo uses Git CLI for fetching. Please append `safe.directory` at the existing count and increment it instead of replacing the list. This can be handled in a follow-up PR. [Git's environment configuration contract](https://git-scm.com/docs/git-config#Documentation/git-config.txt-GITCONFIGCOUNT) ########## spark/src/main/scala/org/apache/comet/rules/CometScanContrib.scala: ########## @@ -0,0 +1,212 @@ +/* + * 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 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, ContribServices} +import org.apache.comet.serde.CometOperatorSerde +import org.apache.comet.util.ClassLoaders Review Comment: [P2] Remove imports left unused by the shared-loader extraction `ClassLoaders` is now used only in a comment here. The same extraction also leaves `java.util.ServiceLoader` and `scala.util.control.NonFatal` unused in `org/apache/spark/sql/comet/operators.scala`. The semanticdb profile enables `-Ywarn-unused`, and the Linux lint workflow runs Scalafix `RemoveUnused` in CHECK mode. A Scala 2.13.16 compiler probe confirms all three unused-import diagnostics; I have not run the full Scalafix workflow. Please remove the imports, particularly if they block the required lint check. -- 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]
