Clean up docs trying to get line count on the diff down.

Author: mbutrovich

docs/source/user-guide/latest/s3-credential-providers.md

@@ -256,7 +256,7 @@ Vendor implementations need the Comet SPI classes at compile time only. Use `pro
 ```xml
 <dependency>
   <groupId>org.apache.datafusion</groupId>
-  <artifactId>comet-common-spark${spark.version.short}_${scala.binary.version}</artifactId>
+  <artifactId>comet-spark-spark${spark.version.short}_${scala.binary.version}</artifactId>
   <version>${comet.version}</version>
   <scope>provided</scope>
 </dependency>

native/core/src/cloud/s3/credential_bridge.rs

@@ -17,23 +17,7 @@
 
 //! JNI bridge to the JVM `CometS3CredentialDispatcher` SPI, exposed as
 //! `object_store::CredentialProvider` (raw Parquet path) and `reqsign_core::ProvideCredential`
-//! (Iceberg via `opendal`).
-//!
-//! ```text
-//!   JVM                                        Native (Rust)
-//!   ---                                        -------------
-//!
-//!   spark.hadoop.fs.s3a.comet.credential       parquet/objectstore/s3.rs (object_store)
-//!     .provider.class                          execution/operators/iceberg_scan.rs (opendal)
-//!         |                                              |
-//!         v                                              v
-//!   CometS3CredentialDispatcher                  CometS3CredentialBridge
-//!         ^                                              |
-//!         |   ensureInitialized -> long handle           |
-//!         +<--- getCredentialsForPath(handle, ...) ------+
-//!         v
-//!   vendor CometS3CredentialProvider -> CometS3Credentials
-//! ```
+//! (Iceberg via `opendal`). See `docs/source/contributor-guide/s3-credential-provider-design.md`.
 
 use crate::execution::operators::ExecutionError;
 use crate::jvm_bridge::{jni_new_global_ref, jni_static_call, JVMClasses};
@@ -61,35 +45,29 @@ use std::time::Duration;
 /// executor from holding a stale credential for the entire job lifetime.
 const DEFAULT_EXPIRY_WHEN_UNKNOWN: Duration = Duration::from_secs(300);
 
-/// Once-per-process latch for the "missing expiry" warning; bridges are per-scan so a per-bridge
-/// latch would log once per scan on the same misbehaving provider.
+/// Once-per-process latch for the "missing expiry" warning. Bridges are per-scan, so a per-bridge
+/// latch would re-log on every scan.
 static WARNED_MISSING_EXPIRY: OnceCell<()> = OnceCell::new();
 
 /// Access intent forwarded to the Java SPI. Ordinal must match the JVM `CometS3AccessMode` enum.
 #[derive(Debug, Clone, Copy)]
 pub enum AccessMode {
     Read = 0,
-    /// No native write path yet; kept so the SPI contract is complete.
     #[allow(dead_code)]
     Write = 1,
 }
 
-/// Per-scan credential provider that delegates to the JVM SPI via JNI.
-///
-/// `handle` is the JVM-allocated identity for the `(provider_class, dispatch_key,
-/// catalog_properties)` triple, returned by `ensureInitialized` at construction. Per-request
-/// calls carry `(handle, bucket, path, mode)`, which lets the JVM disambiguate multi-tenant
-/// providers without re-sending the property bag and saves one JNI string allocation on the hot
-/// path. `bucket` and `path` are immutable for the bridge's lifetime so we cache them as JNI
-/// global refs.
+/// Per-scan credential provider that delegates to the JVM SPI via JNI. `handle` is the JVM-side
+/// identity for the `(provider_class, dispatch_key, catalog_properties)` triple returned by
+/// `ensureInitialized`. `bucket_jstr` / `path_jstr` are interned once at construction to avoid
+/// per-call `new_string` allocations on the hot path.
 pub struct CometS3CredentialBridge {
     provider_class: String,
     dispatch_key: String,
     bucket: String,
     path: String,
     mode: AccessMode,
     handle: i64,
-    /// Cached JNI globals for the two constant String arguments to `getCredentialsForPath`.
     bucket_jstr: Arc<Global<JString<'static>>>,
     path_jstr: Arc<Global<JString<'static>>>,
 }
@@ -108,9 +86,6 @@ impl fmt::Debug for CometS3CredentialBridge {
 }
 
 impl CometS3CredentialBridge {
-    /// Run `ensureInitialized` synchronously and stash the returned handle for the bridge's
-    /// lifetime. `dispatch_key` is the bucket on the Parquet path, the catalog name on the Iceberg
-    /// path. `catalog_properties` is forwarded into the vendor's `initialize(Map)`.
     pub fn new(
         provider_class: impl Into<String>,
         dispatch_key: impl Into<String>,
@@ -232,7 +207,7 @@ fn ensure_initialized(
 }
 
 /// Construct a `java.util.HashMap<String,String>` and populate it. Called once per bridge at
-/// construction (per-scan), so the per-call HashMap/put cost is amortized away from the hot path.
+/// construction, so per-call HashMap/put cost stays off the hot path.
 fn build_java_string_map<'a>(
     env: &mut jni::Env<'a>,
     map: &HashMap<String, String>,

native/core/src/execution/operators/iceberg_scan.rs

@@ -51,10 +51,8 @@ use datafusion_comet_spark_expr::EvalMode;
 use datafusion_physical_expr_adapter::{PhysicalExprAdapter, PhysicalExprAdapterFactory};
 use iceberg::scan::FileScanTask;
 
-/// Iceberg-namespaced activation knob for the `CometS3CredentialProvider` SPI, read from a Spark
-/// catalog's `s3.*` property bag (`spark.sql.catalog.<name>.s3.comet.credential.provider.class`).
-/// Mirrors the Hadoop-style `comet.credential.provider.class` used on the Parquet/object_store
-/// path, but lives here because only the Iceberg path consumes it.
+/// Activation key for the `CometS3CredentialProvider` SPI on the Iceberg path, read from a Spark
+/// catalog's `s3.*` property bag.
 const ICEBERG_PROVIDER_CLASS_PROPERTY: &str = "s3.comet.credential.provider.class";
 
 /// Iceberg table scan operator that uses iceberg-rust to read Iceberg tables.
@@ -71,9 +69,8 @@ pub struct IcebergScanExec {
     /// may contain OAuth tokens, REST `credentials.uri`, and other secrets the credential bridge
     /// needs. Redacted in `Debug` so plan dumps and tracing do not leak credentials.
     catalog_properties: HashMap<String, String>,
-    /// Spark V2 catalog name; forwarded as dispatchKey to the credential bridge so multiple
-    /// catalogs sharing one provider FQCN get isolated provider instances. Empty when the table
-    /// has no catalog identity.
+    /// Spark V2 catalog name; forwarded as dispatchKey to the credential bridge. Empty when the
+    /// table has no catalog identity.
     catalog_name: String,
     /// Pre-planned file scan tasks
     tasks: Vec<FileScanTask>,
@@ -269,10 +266,8 @@ impl IcebergScanExec {
 
 const STORAGE_PROPERTY_PREFIXES: &[&str] = &["s3.", "gcs.", "adls.", "client."];
 
-/// Wires the configured Comet credential provider into opendal's S3 service for this scan, or
-/// returns `None` so opendal falls back to its default credential chain. Iceberg passes its
-/// per-catalog properties (`spark.sql.catalog.<name>.*` after Spark stripping), so the activation
-/// key here is `s3.comet.credential.provider.class` to match Iceberg's `s3.*` namespace.
+/// Wires the configured Comet credential provider into opendal's S3 service, or returns `None`
+/// so opendal falls back to its default credential chain.
 fn build_s3_credential_loader(
     metadata_location: &str,
     catalog_properties: &HashMap<String, String>,
@@ -284,10 +279,8 @@ fn build_s3_credential_loader(
         .get(ICEBERG_PROVIDER_CLASS_PROPERTY)
         .map(|s| s.trim())
         .filter(|s| !s.is_empty())?;
-    // Catalog name scopes provider instances on the JVM dispatcher so two catalogs sharing one
-    // provider class get isolated state. Falls back to the bucket when the table has no catalog
-    // identity (e.g. HadoopTables loaded by raw path), keeping the previous behavior in that
-    // case.
+    // Fall back to the bucket when the table has no catalog identity (e.g. HadoopTables loaded by
+    // raw path).
     let dispatch_key: &str = if catalog_name.is_empty() {
         bucket
     } else {

native/core/src/parquet/objectstore/s3.rs

@@ -79,9 +79,7 @@ pub fn create_store(
         source: "Missing bucket name in S3 URL".into(),
     })?;
 
-    // Parquet path: dispatch_key = bucket (matches the per-bucket override config namespace);
-    // catalog_properties is empty since vendors on the Parquet path read from Hadoop conf, not
-    // catalog props.
+    // Parquet path: catalog_properties is empty; vendors here read from Hadoop conf.
     let empty_props: HashMap<String, String> = HashMap::new();
     let bridge = match lookup_provider_class(configs, bucket) {
         Some(provider_class) => match CometS3CredentialBridge::new(
@@ -322,14 +320,10 @@ pub(super) fn get_config_trimmed<'a>(
     get_config(configs, bucket, property).map(|s| s.trim())
 }
 
-/// Hadoop-style config key (without `fs.s3a.` prefix) naming the vendor `CometS3CredentialProvider`
-/// FQCN. Looked up via [`get_config_trimmed`] so the per-bucket override
-/// (`fs.s3a.bucket.<name>.comet.credential.provider.class`) is honored before the global
-/// (`fs.s3a.comet.credential.provider.class`).
+/// Activation key (without `fs.s3a.` prefix) naming the vendor `CometS3CredentialProvider` FQCN.
+/// Per-bucket override is honored via [`get_config_trimmed`].
 const PROVIDER_CLASS_PROPERTY: &str = "comet.credential.provider.class";
 
-/// Returns the configured `CometS3CredentialProvider` FQCN for the given bucket, or `None` when
-/// no provider is registered. Trims surrounding whitespace and treats an empty string as unset.
 fn lookup_provider_class<'a>(
     configs: &'a HashMap<String, String>,
     bucket: &str,

native/jni-bridge/src/lib.rs

@@ -236,8 +236,6 @@ pub struct JVMClasses<'a> {
     /// `None` if the class is not on the classpath.
     pub comet_udf_bridge: Option<CometUdfBridge<'a>>,
     /// JNI handles for the CometS3CredentialDispatcher SPI and the CometS3Credentials POJO.
-    /// Always present (the classes ship in `comet-common`); whether a vendor provider is actually
-    /// registered is a separate runtime check.
     pub comet_s3_credential_dispatcher: CometS3CredentialDispatcher<'a>,
 }
 

spark/src/main/java/org/apache/comet/cloud/s3/CometS3CredentialContext.java

@@ -24,9 +24,8 @@
 /**
  * Per-request context passed to {@link
  * CometS3CredentialProvider#getCredentialsForPath(CometS3CredentialContext)}. New fields can be
- * added here without breaking vendors compiled against earlier Comet versions, so the SPI method
- * signature does not change when Comet learns to forward additional per-request information (e.g.
- * region, write-target ARN).
+ * added here without changing the SPI method signature, so vendors compiled against earlier
+ * versions stay binary-compatible.
  */
 public final class CometS3CredentialContext {
 

spark/src/main/java/org/apache/comet/cloud/s3/CometS3CredentialProvider.java

@@ -22,50 +22,32 @@
 import java.util.Map;
 
 /**
- * SPI for supplying AWS credentials to Comet's native S3 readers, which bypass Spark's Hadoop S3A
- * code path. Vendors implement this when path-aware or vendor-managed credential mechanisms cannot
- * be reached through the standard parameterless {@code AWSCredentialsProvider.getCredentials()}
- * contract.
+ * SPI for supplying AWS credentials to Comet's native S3 readers, which bypass Hadoop S3A. See the
+ * user guide (operator setup, vendor contract) and the contributor-guide design notes for the
+ * rationale.
  *
- * <p>Vendors register an implementation by setting {@code
- * spark.hadoop.fs.s3a.comet.credential.provider.class} (or the per-bucket form {@code
- * spark.hadoop.fs.s3a.bucket.<name>.comet.credential.provider.class}) for the Parquet path, or
- * {@code spark.sql.catalog.<catalog>.s3.comet.credential.provider.class} for the Iceberg path. The
- * class must have a public no-arg constructor.
- *
- * <p>{@link #initialize(Map)} runs once per Comet-cached instance before any {@link
- * #getCredentialsForPath} call, must be cheap and non-blocking, and may receive secrets in its map.
- * {@link #getCredentialsForPath} may be invoked concurrently from many native worker threads so
- * implementations must be thread-safe; it returns credentials or throws (no fall-through).
- *
- * <p>See the user guide on S3 credential providers for caching, refresh, and multi-tenant isolation
- * guidance.
+ * <p>{@link #getCredentialsForPath} may be invoked concurrently from many native worker threads;
+ * implementations must be thread-safe. It returns credentials or throws (no fall-through).
  */
 public interface CometS3CredentialProvider extends AutoCloseable {
 
   /**
-   * Called once per {@code (FQCN, dispatchKey)} on each executor before any {@link
-   * #getCredentialsForPath} call. The {@code catalogProperties} map carries the full FileIO
-   * property bag for the Iceberg path (including {@code credentials.uri}, OAuth tokens, vendor keys
-   * like {@code tenant-id}) and is empty on the Parquet path. The default no-op keeps Parquet
-   * vendors source-compatible.
+   * Called once per Comet-cached instance before any {@link #getCredentialsForPath} call. Must be
+   * cheap and non-blocking. On the Iceberg path the map carries the unfiltered FileIO bag; on the
+   * Parquet path it is empty.
    *
-   * @param catalogProperties unfiltered FileIO/catalog properties; may contain secrets, do not log
+   * @param catalogProperties may contain secrets, do not log
    */
   default void initialize(Map<String, String> catalogProperties) {}
 
   /**
-   * @param context per-request context (bucket, path, access mode). Fields can be added to {@link
-   *     CometS3CredentialContext} in future Comet releases without changing this method signature,
-   *     so vendors compiled against today's API stay binary-compatible.
    * @return non-null credentials; {@code null} is a contract violation
    */
   CometS3Credentials getCredentialsForPath(CometS3CredentialContext context) throws Exception;
 
   /**
-   * Releases vendor-owned resources (HTTP clients, refresh executors, STS connection pools).
-   * Invoked from a best-effort JVM shutdown hook installed by the dispatcher; default no-op suits
-   * stateless providers. The hook swallows exceptions thrown here.
+   * Invoked from the dispatcher's best-effort JVM shutdown hook. Default no-op suits stateless
+   * providers; override to release HTTP clients, refresh executors, etc.
    */
   @Override
   default void close() throws Exception {}

spark/src/main/java/org/apache/comet/cloud/s3/CometS3Credentials.java

@@ -22,12 +22,9 @@
 import java.util.Objects;
 
 /**
- * Credentials returned by a {@link CometS3CredentialProvider}. Fields are read back over JNI by
- * name, so the field names are part of the cross-language contract.
- *
- * <p>{@code sessionToken} is null for non-STS credentials. {@code expirationEpochMillis} of {@code
- * 0} means "unknown"; the Iceberg path then caps opendal's cache at a short fallback to avoid
- * serving stale credentials for the executor lifetime.
+ * Credentials returned by a {@link CometS3CredentialProvider}. Field names are read back over JNI
+ * by name and are part of the cross-language contract. {@code sessionToken} is null for non-STS
+ * credentials; {@code expirationEpochMillis} of {@code 0} means "unknown".
  */
 public final class CometS3Credentials {
 

…/org/apache/comet/util/ClassLoaders.java …/org/apache/comet/util/ClassLoaders.javacommon/src/main/java/org/apache/comet/util/ClassLoaders.java renamed to spark/src/main/java/org/apache/comet/util/ClassLoaders.java common/src/main/java/org/apache/comet/util/ClassLoaders.java renamed to spark/src/main/java/org/apache/comet/util/ClassLoaders.java

@@ -664,10 +664,8 @@ object IcebergReflection extends Logging {
  * @param catalogProperties
  *   Catalog properties for FileIO (S3 credentials, regions, etc.)
  * @param catalogName
- *   Spark V2 catalog name that loaded this table, if it can be derived. Forwarded as
- *   `dispatchKey` to the native CometS3CredentialBridge so two catalogs sharing one provider FQCN
- *   get isolated provider instances. `None` when the table has no catalog identity (e.g.
- *   HadoopTables loaded by raw path).
+ *   Spark V2 catalog name forwarded as `dispatchKey` to CometS3CredentialBridge. `None` when the
+ *   table has no catalog identity (e.g. HadoopTables loaded by raw path).
  */
 case class CometIcebergNativeScanMetadata(
     table: Any,
@@ -740,20 +738,16 @@ object CometIcebergNativeScanMetadata extends Logging {
   }
 
   /**
-   * Best-effort extraction of the Spark V2 catalog name from an Iceberg `Table`. Iceberg's
-   * `Table.name()` returns `catalog.namespace.table` for tables loaded through a catalog. We
-   * intersect that name against the V2 catalogs Spark has registered so a value like `s3.foo` is
-   * not mistaken for a catalog `s3` when no such catalog exists. Falls back to the dotted-prefix
-   * split when the catalog manager is not reachable or the name does not match. Returns `None`
-   * when the table has no catalog identity (e.g. HadoopTables loaded by raw path) or when
-   * reflection fails.
+   * Extracts the Spark V2 catalog name from an Iceberg `Table`. `Table.name()` returns
+   * `catalog.namespace.table` for tables loaded through a catalog; we intersect against the
+   * registered V2 catalogs so a value like `s3.foo` is not mistaken for a catalog `s3`. Returns
+   * `None` for HadoopTables loaded by raw path or when reflection fails.
    */
   private[iceberg] def deriveCatalogName(table: Any): Option[String] =
     deriveCatalogName(table, registeredCatalogNames _)
 
   /**
-   * Test seam for [[deriveCatalogName(table:Any)]]. The `knownCatalogNames` thunk lets tests
-   * inject a fixed catalog set without bootstrapping a SparkSession.
+   * Test seam that lets tests inject a fixed catalog set without bootstrapping a SparkSession.
    */
   private[iceberg] def deriveCatalogName(
       table: Any,
@@ -773,15 +767,6 @@ object CometIcebergNativeScanMetadata extends Logging {
     }
   }
 
-  /**
-   * Calls Iceberg's public `Table.name()` reflectively. Uses `getMethod` so the interface default
-   * is reachable when a concrete table class does not override it. Matches the pattern used for
-   * `Field.name()` / `Column.name()` elsewhere in this file.
-   *
-   * `name()` is a default method on `org.apache.iceberg.Table`. A thrown exception here means the
-   * classpath is wrong or the object is not actually an Iceberg Table, so log at `warn` to make
-   * it visible. A `null` return is legitimate (anonymous tables) and not noteworthy.
-   */
   private def invokeTableName(table: Any): Option[String] = {
     try {
       table.getClass.getMethod("name").invoke(table) match {
@@ -791,10 +776,8 @@ object CometIcebergNativeScanMetadata extends Logging {
       }
     } catch {
       case e: Exception =>
-        logWarning(
-          s"Iceberg reflection: Table.name() not callable on ${table.getClass.getName}; " +
-            "native S3 credential dispatch will fall back to bucket-keyed isolation: " +
-            s"${e.getMessage}")
+        logWarning(s"Iceberg reflection: Table.name() not callable on ${table.getClass.getName}; " +
+          s"native S3 credential dispatch will fall back to bucket-keyed isolation: ${e.getMessage}")
         None
     }
   }