Copilot commented on code in PR #12414: URL: https://github.com/apache/gravitino/pull/12414#discussion_r3809998242
########## spark-connector/v3.5/spark/src/main/java/org/apache/gravitino/spark/connector/catalog/SparkCatalogs.java: ########## @@ -0,0 +1,85 @@ +/* + * 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.gravitino.spark.connector.catalog; + +import com.google.common.collect.ImmutableMap; +import java.util.Map; +import org.apache.gravitino.spark.connector.glue.GravitinoGlueCatalogSpark35; +import org.apache.gravitino.spark.connector.hive.GravitinoHiveCatalogSpark35; +import org.apache.gravitino.spark.connector.iceberg.GravitinoIcebergCatalogSpark35; +import org.apache.gravitino.spark.connector.jdbc.GravitinoJdbcCatalogSpark35; +import org.apache.gravitino.spark.connector.jdbc.postgresql.GravitinoPostgreSqlCatalogSpark35; + +/** + * The catalogs the Spark 3.5 connector ships. + * + * <p>Every supported Spark version has a class of this name in this package, so the driver plugin + * gets the right table from whichever connector jar is on the classpath, without reading the + * running Spark version. The names are compile-time class references, so a renamed catalog fails + * the build rather than the session, and a jar can only name catalogs it actually contains. + */ +public final class SparkCatalogs { + + /** + * The Paimon catalog is the one entry that cannot be a compile-time reference: Paimon publishes + * no {@code paimon-spark-3.5_2.13} artifact, so the Scala 2.13 build compiles the Paimon package + * out and the class is absent from that jar. Named here rather than in shared code because the + * name is version-specific, and resolved by presence below. + */ + private static final String PAIMON_CATALOG = + "org.apache.gravitino.spark.connector.paimon.GravitinoPaimonCatalogSpark35"; + + private static final Map<SparkCatalogKind, String> CLASS_NAMES = buildClassNames(); + + private SparkCatalogs() {} + + /** + * Returns the Spark catalog class name for each kind of catalog this connector implements. + * + * @return catalog kind to Spark catalog class name + */ + public static Map<SparkCatalogKind, String> classNames() { + return CLASS_NAMES; + } + + private static Map<SparkCatalogKind, String> buildClassNames() { + ImmutableMap.Builder<SparkCatalogKind, String> builder = + ImmutableMap.<SparkCatalogKind, String>builder() + .put(SparkCatalogKind.HIVE, GravitinoHiveCatalogSpark35.class.getName()) + .put(SparkCatalogKind.LAKEHOUSE_ICEBERG, GravitinoIcebergCatalogSpark35.class.getName()) + .put(SparkCatalogKind.GLUE, GravitinoGlueCatalogSpark35.class.getName()) + .put(SparkCatalogKind.JDBC, GravitinoJdbcCatalogSpark35.class.getName()) + .put( + SparkCatalogKind.JDBC_POSTGRESQL, + GravitinoPostgreSqlCatalogSpark35.class.getName()); + if (isPresent(PAIMON_CATALOG)) { + builder.put(SparkCatalogKind.LAKEHOUSE_PAIMON, PAIMON_CATALOG); + } + return builder.build(); + } + + private static boolean isPresent(String className) { + try { + Class.forName(className, false, SparkCatalogs.class.getClassLoader()); + return true; + } catch (ClassNotFoundException e) { + return false; + } Review Comment: `Class.forName(..., false, ...)` can still throw `NoClassDefFoundError` while loading this class's Paimon superclasses/interfaces. The connector runtime does not bundle Paimon (`compileOnly`), and the documented default is to add that runtime only when Paimon is enabled, so a normal Spark 3.5 deployment without Paimon can fail while initializing `CLASS_NAMES` even when it only uses Hive/JDBC. The tests mask this by adding Paimon to `testImplementation`; treat the missing transitive types as “not present” too. ########## spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/utils/HiveGravitinoOperationOperator.java: ########## @@ -30,15 +30,19 @@ import org.apache.gravitino.rel.partitions.Partition; import org.apache.gravitino.rel.partitions.Partitions; import org.apache.spark.sql.catalyst.InternalRow; -import org.apache.spark.sql.catalyst.analysis.PartitionAlreadyExistsException; +import org.apache.spark.sql.catalyst.analysis.PartitionsAlreadyExistException; import org.apache.spark.sql.catalyst.expressions.GenericInternalRow; import org.apache.spark.sql.types.DataType; import org.apache.spark.sql.types.StructField; import org.apache.spark.sql.types.StructType; import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class HiveGravitinoOperationOperator { + private static final Logger LOG = LoggerFactory.getLogger(HiveGravitinoOperationOperator.class); + private org.apache.gravitino.rel.Table gravitinoTable; private static final String PARTITION_NAME_DELIMITER = "/"; private static final String PARTITION_VALUE_DELIMITER = "="; Review Comment: The instance field now precedes two static constants, violating the repository's required class-member order (all static constants/fields before instance fields). Move both delimiter constants above `gravitinoTable`. ########## spark-connector/v4.0/spark/src/main/java/org/apache/gravitino/spark/connector/iceberg/GravitinoIcebergCatalogSpark40.java: ########## @@ -0,0 +1,61 @@ +/* + * 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.gravitino.spark.connector.iceberg; + +import java.lang.reflect.InvocationTargetException; +import org.apache.iceberg.spark.procedures.SparkProcedures; +import org.apache.spark.sql.connector.catalog.Identifier; +import org.apache.spark.sql.connector.catalog.ProcedureCatalog; +import org.apache.spark.sql.connector.catalog.procedures.UnboundProcedure; + +/** + * Spark 4.0 specific Gravitino Iceberg catalog implementation. {@link ProcedureCatalog} is declared + * here rather than in the shared base because Spark 4 moved the procedure types out of Iceberg's + * package into its own and changed {@code loadProcedure} to return {@link UnboundProcedure}. + */ +public class GravitinoIcebergCatalogSpark40 extends GravitinoIcebergCatalog + implements ProcedureCatalog { + + /** + * Procedures validate that the catalog registered with Spark's catalogManager is the same one + * passed to the {@code ProcedureBuilder} that invokes loadProcedure(). Pass this catalog rather + * than the internal Spark catalog to satisfy that check. + */ + @Override + public UnboundProcedure loadProcedure(Identifier identifier) { + String[] namespace = identifier.namespace(); + String name = identifier.name(); + + try { + if (isSystemNamespace(namespace)) { + SparkProcedures.ProcedureBuilder builder = SparkProcedures.newBuilder(name); + if (builder != null) { + return builder.withTableCatalog(this).build(); + } + } + } catch (NoSuchMethodException + | IllegalAccessException + | InvocationTargetException + | ClassNotFoundException e) { + throw new RuntimeException("Failed to load Iceberg Procedure " + identifier, e); + } + + throw new RuntimeException("Procedure does not exist: " + identifier); Review Comment: Spark's `ProcedureCatalog` contract uses `NoSuchProcedureException` for an unknown procedure. A bare `RuntimeException` here makes `CALL` on an unknown or non-system procedure surface as an internal runtime failure instead of Spark's standard analysis error. Throw `NoSuchProcedureException(identifier)`, as the Spark 3.5 implementation does. ########## docs/spark-connector/spark-connector.md: ########## @@ -17,13 +17,13 @@ The Apache Gravitino Spark connector leverages the Spark DataSourceV2 interface ## Requirement -* Spark 3.3 or 3.4 or 3.5 -* Scala 2.12 or 2.13 -* JDK 8, 11 or 17 +* Spark 3.5 or 4.0 +* Scala 2.12 or 2.13 on Spark 3.5; Spark 4 is Scala 2.13 only +* JDK 8, 11 or 17 on Spark 3.5; Spark 4 requires JDK 17 ## Usage -1. [Build](../how-to-build.md) or download the package ([gravitino-spark-connector-runtime-3.3](https://mvnrepository.com/artifact/org.apache.gravitino/gravitino-spark-connector-runtime-3.3), [gravitino-spark-connector-runtime-3.4](https://mvnrepository.com/artifact/org.apache.gravitino/gravitino-spark-connector-runtime-3.4), [gravitino-spark-connector-runtime-3.5](https://mvnrepository.com/artifact/org.apache.gravitino/gravitino-spark-connector-runtime-3.5)), and place it to the classpath of Spark. +1. [Build](../how-to-build.md) or download the package matching your Spark minor version ([gravitino-spark-connector-runtime-3.5](https://mvnrepository.com/artifact/org.apache.gravitino/gravitino-spark-connector-runtime-3.5), [gravitino-spark-connector-runtime-4.0](https://mvnrepository.com/artifact/org.apache.gravitino/gravitino-spark-connector-runtime-4.0)), and place it to the classpath of Spark. Review Comment: The new link omits the Scala suffix, but this PR publishes the artifact as `gravitino-spark-connector-runtime-4.0_2.13` (see the runtime module's `baseName`). The current URL therefore does not point to the published Spark 4 artifact. -- 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]
