vinishjail97 commented on code in PR #843: URL: https://github.com/apache/incubator-xtable/pull/843#discussion_r3617459092
########## .github/workflows/spark-runtime-validation.yml: ########## @@ -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. +# +name: Spark Runtime Bundle Validation + +# Validates the shaded xtable-spark-runtime bundle end-to-end: ITXTableSparkRuntimeBundle +# spark-submits the bundled jar against a real Spark distribution for one case per direction +# across Hudi, Iceberg and Delta. The IT self-skips unless SPARK_HOME is set, so the main CI +# never exercises it; this workflow installs a matching Spark 3.4 distro and runs it. + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ !contains(github.ref, 'main') }} + +# Runs on every push/PR to the covered branches (no path filter) so the check always +# reports — required status checks that never trigger leave a PR blocked on a pending check. +on: + push: + branches: + - "main" + - "branch-0.4" + + pull_request: + branches: + - "main" + - "branch-0.4" + + workflow_dispatch: + +env: + SPARK_VERSION: "3.4.3" Review Comment: Thanks for the question. It is pinned to the Spark 3.4 line because the engine set is Spark-3.4-bound: delta-core 2.4.0 is Spark-3.4-only (Spark 3.5 needs Delta 3.x) and the Hudi/Iceberg runtimes are published per Spark minor, so moving to 3.5 is a coordinated engine-version bump rather than a one-line change. I added a comment explaining this and a workflow_dispatch input to override the version within the 3.4 line (91903423). ########## .github/workflows/spark-runtime-validation.yml: ########## @@ -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. +# +name: Spark Runtime Bundle Validation + +# Validates the shaded xtable-spark-runtime bundle end-to-end: ITXTableSparkRuntimeBundle +# spark-submits the bundled jar against a real Spark distribution for one case per direction +# across Hudi, Iceberg and Delta. The IT self-skips unless SPARK_HOME is set, so the main CI +# never exercises it; this workflow installs a matching Spark 3.4 distro and runs it. + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ !contains(github.ref, 'main') }} + +# Runs on every push/PR to the covered branches (no path filter) so the check always +# reports — required status checks that never trigger leave a PR blocked on a pending check. +on: + push: + branches: + - "main" + - "branch-0.4" + + pull_request: + branches: + - "main" + - "branch-0.4" + + workflow_dispatch: + +env: + SPARK_VERSION: "3.4.3" + SPARK_HADOOP_PROFILE: "hadoop3" + +jobs: + validate-spark-runtime-bundle: + runs-on: ubuntu-latest + timeout-minutes: 30 + steps: + - uses: actions/checkout@v6 + + - name: Set up JDK 11 + uses: actions/setup-java@v5 + with: + java-version: '11' + distribution: 'temurin' + cache: maven + + - name: Cache Spark distribution + uses: actions/cache@v4 + with: + path: ~/spark + key: spark-${{ env.SPARK_VERSION }}-bin-${{ env.SPARK_HADOOP_PROFILE }} + + - name: Install Spark + run: | + set -euo pipefail + SPARK_DIR="spark-${SPARK_VERSION}-bin-${SPARK_HADOOP_PROFILE}" Review Comment: Thanks for thinking about this. Spark is already cached - the "Cache Spark distribution" step (actions/cache keyed on the version and hadoop profile) restores ~/spark so the download is skipped on a cache hit, and Maven dependencies are cached via setup-java, so Spark is not reinstalled each run. ########## .github/workflows/mvn-ci-build.yml: ########## @@ -24,10 +24,12 @@ on: push: branches: - "main" + - "branch-0.4" Review Comment: Thanks. Yes - branch-0.4 is the 0.4.x maintenance branch where fixes are cherry-picked for future 0.4.x releases, which is why this bundle targets it rather than main. ########## xtable-core/src/main/java/org/apache/xtable/conversion/ConversionTargetFactory.java: ########## @@ -96,7 +119,10 @@ && isDeltaKernelTarget(target) == useKernel) { throw new NotSupportedException("Target format is not yet supported: " + tableFormatName); } + private static final String DELTA_KERNEL_TARGET_CLASS = + "org.apache.xtable.kernel.DeltaKernelConversionTarget"; + private static boolean isDeltaKernelTarget(ConversionTarget target) { - return target instanceof DeltaKernelConversionTarget; + return DELTA_KERNEL_TARGET_CLASS.equals(target.getClass().getName()); Review Comment: Good question, thank you. You are right that a rename would not fail at compile time, since it is a runtime ServiceLoader lookup by format string. It is caught at runtime instead: the spark-runtime IT exercises all three formats end-to-end, so a broken or renamed provider registration fails there. Happy to add a targeted assertion if you would like a tighter guard. ########## xtable-spark-runtime/src/main/java/org/apache/xtable/spark/XTableSparkSync.java: ########## @@ -0,0 +1,165 @@ +/* + * 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.xtable.spark; + +import java.util.Arrays; +import java.util.List; +import java.util.Locale; +import java.util.stream.Collectors; + +import lombok.extern.log4j.Log4j2; + +import org.apache.commons.cli.CommandLine; +import org.apache.commons.cli.CommandLineParser; +import org.apache.commons.cli.DefaultParser; +import org.apache.commons.cli.HelpFormatter; +import org.apache.commons.cli.Option; +import org.apache.commons.cli.Options; +import org.apache.commons.cli.ParseException; +import org.apache.hadoop.conf.Configuration; +import org.apache.spark.sql.SparkSession; + +/** + * Standalone {@code spark-submit} entry point that runs a single XTable sync using the relocated + * {@code xtable-spark-runtime} bundle. It is the RunSync-equivalent for this bundle and is + * primarily used to validate that the shaded jar is self-contained on a real Spark install. + * + * <pre> + * $SPARK_HOME/bin/spark-submit \ + * --class org.apache.xtable.spark.XTableSparkSync \ + * xtable-spark-runtime_2.12-<ver>.jar \ + * --basePath /warehouse/db/orders --sourceFormat HUDI --targets ICEBERG,DELTA + * </pre> + */ +@Log4j2 +public final class XTableSparkSync { + + private static final String BASE_PATH = "basePath"; + private static final String DATA_PATH = "dataPath"; + private static final String SOURCE_FORMAT = "sourceFormat"; + private static final String TARGETS = "targets"; + private static final String TABLE_NAME = "tableName"; + private static final String NAMESPACE = "namespace"; + private static final String PARTITION_SPEC = "partitionSpec"; + private static final String HELP = "help"; + + private static final Options OPTIONS = + new Options() + .addOption( + Option.builder() + .longOpt(BASE_PATH) + .hasArg() + .required() + .desc("The base path of the source table") + .build()) + .addOption( + Option.builder() + .longOpt(SOURCE_FORMAT) Review Comment: Thanks for the question. longOpt is commons-cli's term for the long-form option name (the --sourceformat spelling, as opposed to a short -s flag), not a numeric long. Formats come from TableFormat, which is a holder of String constants rather than a Java enum, so there is no int/short mapping. The value is now validated up front against the supported formats (91903423). ########## xtable-spark-runtime/src/main/resources/META-INF/NOTICE-bundled: ########## @@ -0,0 +1,22 @@ +Apache XTable (incubating) +Copyright 2024-2026 The Apache Software Foundation + +This product includes software developed at +The Apache Software Foundation (https://www.apache.org/). + +-------------------------------------------------------------------------------- + +This binary artifact bundles the following projects with NOTICE: + +-------------------------------------------------------------------------------- + +Group: commons-cli Name: commons-cli Version: 1.5.0 Review Comment: Thanks for flagging - commons version mismatches are a real pain. The only commons library the bundle ships is commons-cli, and it is relocated to org.apache.xtable.shaded.org.apache.commons.cli, so it cannot clash with Hadoop/Spark's commons. Everything else (avro, parquet, jackson, other commons-*) is provided by the cluster and never bundled, so there is no second copy to mismatch. ########## xtable-spark-runtime/pom.xml: ########## @@ -0,0 +1,382 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ 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. +--> +<project xmlns="http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <parent> + <groupId>org.apache.xtable</groupId> + <artifactId>xtable</artifactId> + <version>0.2.0-SNAPSHOT</version> + </parent> + + <artifactId>xtable-spark-runtime_${scala.binary.version}</artifactId> + <name>XTable Project Spark Runtime</name> + <description> + Thin, relocated, drop-in Spark bundle that runs incremental XTable metadata sync. v1 ships + the standalone spark-submit entry point (XTableSparkSync); the config-only, driver-side + QueryExecutionListener on-ramp is a planned follow-up (see rfc/rfc-3). Review Comment: Thanks. Yes - v1 is the manual spark-submit CLI (XTableSparkSync). The config-only, driver-side auto-sync listener is deferred to a follow-up PR, as noted in the RFC's "What ships in v1" section. ########## rfc/rfc-3/rfc-3.md: ########## @@ -0,0 +1,234 @@ +<!-- + 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. +--> +# RFC-3: xtable-spark-runtime - in-job metadata sync via a thin Spark bundle + +## Proposers + +- @vinishjail97 + +## Approvers +- Anyone from the XTable community can approve/add feedback. + +## Status Review Comment: Thank you for the review, much appreciated. I have addressed your inline comments in 91903423 (CLI validation and unix-style flags, the richer skip-log, and the Spark-version pin explanation) and updated the RFC (supported-formats matrix, from-application-code examples, and approver). -- 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]
