Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue #1908

gh-yzou · 2025-06-18T18:21:52Z

We run into an issue when testing the spark client using --packages (the jar path works correctly), where the iceberg requires avro 1.12.0, but the one provide by spark is 1.11.4. Even though the dependency is downloaded, the one used is still the one provided by spark. There for we see error like following when do select * from iceberg_tb:

java.lang.NoSuchMethodError: 'org.apache.avro.LogicalTypes$TimestampNanos org.apache.avro.LogicalTypes.timestampNanos()'
  at org.apache.iceberg.avro.TypeToSchema.<clinit>(TypeToSchema.java:50)
  at org.apache.iceberg.avro.AvroSchemaUtil.convert(AvroSchemaUtil.java:64)
  at org.apache.iceberg.avro.AvroSchemaUtil.convert(AvroSchemaUtil.java:59)
  at org.apache.iceberg.GenericManifestFile.<clinit>(GenericManifestFile.java:42)
  at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
  at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:75)
  at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:53)
  at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
  at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)
  at org.apache.iceberg.common.DynConstructors$Ctor.newInstanceChecked(DynConstructors.java:51)
  at org.apache.iceberg.common.DynConstructors$Ctor.newInstance(DynConstructors.java:64)
  at org.apache.iceberg.avro.InternalReaders$PlannedStructLikeReader.reuseOrCreate(InternalReaders.java:67)
  at org.apache.iceberg.avro.InternalReaders$PlannedStructLikeReader.reuseOrCreate(InternalReaders.java:42)
  at org.apache.iceberg.avro.ValueReaders$PlannedStructReader.read(ValueReaders.java:963)
  at org.apache.iceberg.avro.InternalReader.read(InternalReader.java:107)
  at org.apache.iceberg.avro.NameMappingDatumReader.read(NameMappingDatumReader.java:57)
  at org.apache.avro.file.DataFileStream.next(DataFileStream.java:263)
  at org.apache.avro.file.DataFileStream.next(DataFileStream.java:248)

A quick mitigation is to remove the default one used by spark. However, to avoid future conflict with other library, we switch to use the shaded library shipped in iceberg-spark-runtime.

The major change is that instead using com.fasterxml.jackson.annotation, we uses org.apache.iceberg.shaded.com.fasterxml.jackson.annotation for all request and response to ensure they can be serialized and deserialized correctly by Iceberg RESTClient.
Since the classes now looks different than the one used at server side now, we actually let client uses its own request and response classes. Those classes are added manually at this moment, will follow up to autogenerate them based on spec directly #1909

Since we are using all libraries shipped along with the iceberg-spark-runtime, it simplifies the build for client significantly

dimas-b · 2025-06-18T18:26:05Z

plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/PolarisSparkCatalog.java

This change is fine from my POV, but it does not seem to match the PR title and description. Please mention this refactoring there.

because the classes used by client and server now are different in terms of the import (server uses the regular import, but client uses the shaded one coming from iceberg). So the implementation of the class at client side are now different compare with server side, and the client side is now suppose to use the one provided at client side. This change is to use the class provided by the client side, which will be auto generated in the coming task.

Move on, the only contract client have with server will be the API spec, which i think is the correct thing to do.

I added a comment here, and also updated the description to contain more details

plugins/spark/v3.5/integration/build.gradle.kts

dimas-b · 2025-06-18T18:32:14Z

plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/rest/GenericTable.java

+1 to the TODO, but as of now this class is checked in to the source repo, not generated at build time 🤔

yes, this class requires a different code generation script because the code looks different compare with the one generated by server side in terms of the import. where the server side uses

import com.fasterxml.jackson.annotation.JsonCreator

but we will need to use

import org.apache.iceberg.shaded.com.fasterxml.jackson.annotation.JsonCreator;

at client side to enable reuse of iceberg rest client correctly, because the RESTClient shipped in the iceberg-spark-runtime has the jackson library shaded. i will have to add new generation script and build file to make sure things can be generated properly, and I have an issue to track the improvement #1909

I mean: do we need the @Generated annotation here? The file appears to be modified after generation and manually committed to git 🤔

i see. yes, we do not need the @generated annotation here, removed.

codestyle/checkstyle.xml

flyrain · 2025-06-20T16:00:25Z

...ntegration/src/intTest/java/org/apache/polaris/spark/quarkus/it/PolarisManagementClient.java

Nit:

Suggested change

/**

* That class provides rest client that is can be used to talk to Polaris Management service and

* auth token endpoint. This class is currently used by Spark Client tests for commands that can not

* be issued through spark command, such as createCatalog etc.

*/

/**

* This class provides a REST client for the Polaris Management service endpoints and its auth-token endpoint, which used in

* Spark client tests to run commands that Spark SQL can’t issue directly (e.g., createCatalog).

*/

flyrain · 2025-06-20T16:03:25Z

plugins/spark/v3.5/integration/build.gradle.kts

+1 this provides more flexibility on the client side by minimizing dependencies, as it only depends on the Polaris REST spec now.

flyrain

Thanks a lot @gh-yzou for the fix! LGTM!

flyrain · 2025-06-20T16:08:23Z

A minor suggestion, would you mind sharing the error msg/stack trace of the issue the PR trying to fix? This will provide more context people who track this issue.

dimas-b · 2025-06-20T16:19:33Z

I raised some concerns about this PR on the dev ML: https://lists.apache.org/thread/0z30f3cfvm41hxlbxgp4fqdpv7mfgnv8

Let's resolve that discussion before merging.

dimas-b · 2025-06-20T16:21:04Z

codestyle/checkstyle_no_illegalimport.xml

I'm not sure this approach is ideal for making exceptions to Checkstyle rules.

@adutra : WDYT?

switched to use suppress rule now

flyrain · 2025-06-20T17:22:04Z

plugins/spark/v3.5/spark/build.gradle.kts

cc @dimas-b, it doesn't depend on iceberg-core anymore, not leaking of transitive dependencies.

gh-yzou · 2025-06-20T21:38:46Z

@flyrain i added the stacktrace in the PR description.

dimas-b · 2025-06-20T22:24:46Z

codestyle/checkstyle.xml

Is this not the same as lines 41-42?

it is not the same, actually line 41-42 is not doing anything, the config says look for config org.checkstyle.google.suppressionfilter.config, which i don't think we configure it anywhere in our project, if not configured, use file checkstyle-suppressions.xml, which we do not have it anywhere in the project also, the check style job doesn't fail, because it sets optional to true.
Now since we do have the suppressions file, i made it non-optional, and removed the other configuration. One thing i did notice here is that if i just use the relative path regarding to root project path, gradlew build seems not able to found the path, the recommended way seems using the config, and then get the absolute path, which is the way i am going with now

i moved the suppress file to the polaris-spark project, and reuses the original configuration

dimas-b · 2025-06-20T22:25:57Z

codestyle/checkstyle_suppressions.xml

Is it possible to keep this file under the Spark module as opposed to the global codestyle dir?

Actually, i think it make sense to have it at the top level, in the future, if there are other suppress rule we want to add, they can all be added to this file, and we will have centralized file to mange all checkstyle and suppress rule, which i think can make things much easier to mange.

Agree with @dimas-b , let's not add module specifics here globally - that does affect all modules

Moved the suppress config to spark client side.

snazy · 2025-06-23T13:12:57Z

build-logic/src/main/kotlin/polaris-java.gradle.kts

Please do not add a project specific change to all projects?

moved to project specific

snazy · 2025-06-23T13:14:33Z

codestyle/checkstyle_suppressions.xml

Agree with @dimas-b , let's not add module specifics here globally - that does affect all modules

snazy · 2025-06-23T13:24:35Z

plugins/spark/v3.5/integration/build.gradle.kts

Why have two Jackson in tests - the relocated one and this?

That one is used by a polaris client test utility that is used to talk to the polaris management API for test setup, such as createCatalog. This test utility has nothing to do with Iceberg, and is not suppose to rely on the iceberg spark client library.

…ache#1857)" This reverts commit 1f7f127.

* fix spark client * fix test failure and address feedback * fix error * update regression test * update classifier name * address comment * add change * update doc * update build and readme * add back jr * udpate dependency * add change * update * update tests * remove merge service file * update readme * update readme

…ache#1857)" This reverts commit 40f4d36.

flyrain

Thanks for keeping working on it, @gh-yzou !

dimas-b

I still think that compiling against shaded jackson classes is sub-optimal and can be avoided, but I will not hold this PR because of that. Other changes LGTM.

…untime to avoid spark compatibilities issue (#1908) * add change * add comment * update change * add comment * add change * add tests * add comment * clean up style check * update build * Revert "Reuse shadowJar for spark client bundle jar maven publish (#1857)" This reverts commit 1f7f127. * Reuse shadowJar for spark client bundle jar maven publish (#1857) * fix spark client * fix test failure and address feedback * fix error * update regression test * update classifier name * address comment * add change * update doc * update build and readme * add back jr * udpate dependency * add change * update * update tests * remove merge service file * update readme * update readme * update checkstyl * rebase with main * Revert "Reuse shadowJar for spark client bundle jar maven publish (#1857)" This reverts commit 40f4d36. * update checkstyle * revert change * address comments * trigger tests

* Cleanup unnecessary files in client/python (apache#1878) Cleanup unnecessary files in `client/python` * Bump version in version.txt With the release/1.0.0 branch being cut, we should bump this to reflect the current state of main * JDBC: Refactor DatabaseOps (apache#1843) * removes the databaseType computation from JDBCMetastoreManagerFactory to DbOperations * wraps the bootstrap in a transaction ! * refactor Production Readiness checks for Postgres * Fix two wrong links in README.md (apache#1879) * Avoid using org.testcontainers.shaded.** (apache#1876) * main: Update dependency io.smallrye.config:smallrye-config-core to v3.13.2 (apache#1888) * main: Update registry.access.redhat.com/ubi9/openjdk-21-runtime Docker tag to v1.22-1.1749462970 (apache#1887) * main: Update dependency boto3 to v1.38.36 (apache#1886) * fix(build): Fix deprecation warnings in PolarisIntegrationTestExtension (apache#1895) * Enable patch version updates for maintained Polaris version (apache#1891) Polaris 1.x will be a supported/maintained release. It is crucial to apply bug and security fixes to such release branches. Therefore, this change enables patch-version updates for Polaris 1.* * Add Polaris community meeting record for 2025-06-12 (apache#1892) * Do not use relative path inside CLI script Issue apache#1868 reported that the Polaris script can fail when it's run from an unexpected path. The recent addition of a reference to `./gradlew` looks incorrect here, and should be changed to use an absolute path. Fixes apache#1868 * feat(build): Add Checkstyle plugin and an IllegalImport rule (apache#1880) * Python CI: pin mypy version to avoid CI failure due to new release (apache#1903) Mypy did a new release 1.16.1 and it cause our CI to fail for about 20 minutes due to missing wheel (upload not completed) ``` | Unable to find installation candidates for mypy (1.16.1) | | This is likely not a Poetry issue. | | - 14 candidate(s) were identified for the package | - 14 wheel(s) were skipped as your project's environment does not support the identified abi tags | | Solutions: | Make sure the lockfile is up-to-date. You can try one of the following; | | 1. Regenerate lockfile: poetry lock --no-cache --regenerate | 2. Update package : poetry update --no-cache mypy | | If neither works, please first check to verify that the mypy has published wheels available from your configured source that are compatible with your environment- ie. operating system, architecture (x86_64, arm64 etc.), python interpreter. | ``` This PR temporarily restrict the mypy version to avoid the similar issue. We may consider bring poetry.lock back to git tracking so we won't automatically update test dependencies all the time * Remove `.github/CODEOWNERS` (apache#1902) As per this [dev-ML discussion](https://lists.apache.org/thread/jjr5w3hslk755yvxy8b3z45c7094cxdn) * Rename quarkus as runtime (apache#1695) * Rename runtime/test-commons to runtime/test-common (for consistency with module name) (apache#1906) * docs: Add `Polaris Evolution` page (apache#1890) --------- Co-authored-by: Eric Maynard <[email protected]> * feat(ci): Split Java Gradle CI in many jobs to reduce execution time (apache#1897) * Add webpage for Generic Table support (apache#1889) * add change * add comment * address feedback * update limitations * update docs * update doc * address feedback * Improve the parsing and validation of UserSecretReferenceUrns (apache#1840) This change addresses all the TODOs found the org.polaris.core.secrets package. Main changes: - Create a helper to parse, validate and build the URN strings. - Use Regex instead of `String.split()`. - Add Precondition checks to ensure that the URN is valid and the UserSecretManager matches the expected type. - Remove the now unused `GLOBAL_INSTANCE` of the UnsafeInMemorySecretsManager. Testing - Existing `UnsafeInMemorySecretsManagerTest` captures most of the functional changes. - Added `UserSecretReferenceUrnHelperTest` to capture the utilities exposed. * Reuse shadowJar for spark client bundle jar maven publish (apache#1857) * fix spark client * fix test failure and address feedback * fix error * update regression test * update classifier name * address comment * add change * update doc * update build and readme * add back jr * udpate dependency * add change * update * update tests * remove merge service file * update readme * update readme * fix(ci): Remove dummy "build" job from Gradle CI (apache#1911) Since apache#1897, the jobs in gradle.yaml changed and the "build" job was split into many smaller jobs. But since it was a required job, it couldn't be removed immediately. * main: Update Quarkus Platform and Group to v3.23.3 (apache#1797) * main: Update Quarkus Platform and Group to v3.23.3 * Adopt polaris-admin test invocation --------- Co-authored-by: Robert Stupp <[email protected]> * Feature: Rollback compaction on conflict (apache#1285) Intention is make the catalog smarter, to revert the compaction commits in case of crunch to let the writers who are actually adding or removing the data to the table succeed. In a sense treating compaction as always a lower priority process. Presently the rest catalog client creates the snapshot and asks the Rest Server to apply the snapshot and gives this in a combination of requirement and update. Polaris could apply some basic inference and generate some updates to metadata given a property is enabled at a table level, by saying that It will revert back the commit which was created by compaction and let the write succeed. I had this PR in OSS, which was essentially doing this at the client end, but we think its best if we do this as server end. to support more such clients. How to use this Enable a catalog level configuration : polaris.config.rollback.compaction.on-conflicts.enabled when this is enabled polaris will apply the intelligence of rollbacking those REPLACE ops snapshot which have the property of polaris.internal.rollback.compaction.on-conflict in their snapshot summary to resolve conflicts at the server end ! a sample use case is there is a deployment of a Polaris where this config is enabled and there is auto compaction (maintenance job) which is updating the table state, it adds the snapshot summary that polaris.internal.rollback.compaction.on-conflict is true now when a backfill process running for 8 hours want to commit but can't because the compaction job committed before so in this case it will reach out to Polaris and Polaris will see if the snapshot of compation aka replace snapshot has this property if yes roll it back and let the writer succeed ! Devlist: https://lists.apache.org/thread/8k8t77dgk1vc124fnb61932bdp9kf1lc * NoSQL: nits * `AutoCloseable` for `PersistenceTestExtension` * checkstyle adoptions * fix: unify bootstrap credentials and standardize POLARIS setup (apache#1905) - unified formatting across docker, gradle - reverted secret to s3cr3t - updated docker-compose, README, conftest.py use POLARIS for consistency across docker, gradle and others. * Add doc for rollback config (apache#1919) * Revert "Reuse shadowJar for spark client bundle jar maven publish (apache#1857)" (apache#1921) …857)" This reverts commit 1f7f127. The shadowJar plugin actually stops publish the original jar, which is not what spark client intend to publish for the --package usage. Revert it for now, will follow up with a better way to reuse the shadow jar plugin, likely with a separate bundle project * fix(build): Gradle caching effectively not working (apache#1922) Using a `custom()` spotless formatter check effectively disables caching, see `com.diffplug.gradle.spotless.FormatExtension#custom(java.lang.String, com.diffplug.spotless.FormatterFunc)` using `globalState`, which is a `NeverUpToDateBetweenRuns`. This change refactors this to be cachable. We also already have a errorprone rule, so we can get rid entirely of the spotless step. * Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue (apache#1908) * add change * add comment * update change * add comment * add change * add tests * add comment * clean up style check * update build * Revert "Reuse shadowJar for spark client bundle jar maven publish (apache#1857)" This reverts commit 1f7f127. * Reuse shadowJar for spark client bundle jar maven publish (apache#1857) * fix spark client * fix test failure and address feedback * fix error * update regression test * update classifier name * address comment * add change * update doc * update build and readme * add back jr * udpate dependency * add change * update * update tests * remove merge service file * update readme * update readme * update checkstyl * rebase with main * Revert "Reuse shadowJar for spark client bundle jar maven publish (apache#1857)" This reverts commit 40f4d36. * update checkstyle * revert change * address comments * trigger tests * Last merged commit 93938fd --------- Co-authored-by: Honah (Jonas) J. <[email protected]> Co-authored-by: Eric Maynard <[email protected]> Co-authored-by: Prashant Singh <[email protected]> Co-authored-by: Yufei Gu <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: JB Onofré <[email protected]> Co-authored-by: Eric Maynard <[email protected]> Co-authored-by: Yun Zou <[email protected]> Co-authored-by: Pooja Nilangekar <[email protected]> Co-authored-by: Seungchul Lee <[email protected]>

github-project-automation bot added this to Basic Kanban Board Jun 18, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jun 18, 2025

gh-yzou requested review from RussellSpitzer, adutra, eric-maynard and flyrain June 18, 2025 18:21

dimas-b reviewed Jun 18, 2025

View reviewed changes

plugins/spark/v3.5/integration/build.gradle.kts Outdated Show resolved Hide resolved

dimas-b reviewed Jun 18, 2025

View reviewed changes

codestyle/checkstyle.xml Outdated Show resolved Hide resolved

gh-yzou force-pushed the yzou-test-shade branch from 627bd0d to df7bcd2 Compare June 18, 2025 21:55

flyrain reviewed Jun 20, 2025

View reviewed changes

flyrain previously approved these changes Jun 20, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jun 20, 2025

dimas-b reviewed Jun 20, 2025

View reviewed changes

flyrain reviewed Jun 20, 2025

View reviewed changes

gh-yzou dismissed flyrain’s stale review via f122578 June 20, 2025 20:53

gh-yzou force-pushed the yzou-test-shade branch from 22a1738 to f122578 Compare June 20, 2025 20:53

dimas-b reviewed Jun 20, 2025

View reviewed changes

snazy reviewed Jun 23, 2025

View reviewed changes

gh-yzou added 5 commits June 23, 2025 11:08

add change

947135e

add comment

1c0255f

update change

32804fc

add comment

691f149

add change

5494d1e

gh-yzou added 10 commits June 23, 2025 11:08

add tests

469dcce

add comment

89cb53d

clean up style check

85a8789

update build

783ad75

Revert "Reuse shadowJar for spark client bundle jar maven publish (ap…

e512399

…ache#1857)" This reverts commit 1f7f127.

update checkstyl

d44f87f

rebase with main

328889a

Revert "Reuse shadowJar for spark client bundle jar maven publish (ap…

82f31e7

…ache#1857)" This reverts commit 40f4d36.

update checkstyle

69a7c69

gh-yzou force-pushed the yzou-test-shade branch from 7571e96 to 69a7c69 Compare June 23, 2025 19:12

gh-yzou added 3 commits June 23, 2025 12:14

revert change

c1651dd

address comments

d8e2b4c

trigger tests

cafb710

flyrain approved these changes Jun 23, 2025

View reviewed changes

dimas-b reviewed Jun 23, 2025

View reviewed changes

gh-yzou merged commit 93938fd into apache:main Jun 23, 2025
11 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Jun 23, 2025

-/**
- * That class provides rest client that is can be used to talk to Polaris Management service and
- * auth token endpoint. This class is currently used by Spark Client tests for commands that can not
- * be issued through spark command, such as createCatalog etc.
- */
+/**
+ * This class provides a REST client for the Polaris Management service endpoints and its auth-token endpoint, which used in
+ * Spark client tests to run commands that Spark SQL can’t issue directly (e.g., createCatalog).
+ */

Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue #1908

Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue #1908

Uh oh!

Conversation

gh-yzou commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

flyrain commented Jun 20, 2025

Uh oh!

dimas-b commented Jun 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gh-yzou commented Jun 20, 2025

Uh oh!

dimas-b Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gh-yzou Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gh-yzou Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gh-yzou Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

gh-yzou commented Jun 18, 2025 •

edited

Loading

dimas-b Jun 20, 2025 •

edited

Loading

gh-yzou Jun 20, 2025 •

edited

Loading

gh-yzou Jun 20, 2025 •

edited

Loading

gh-yzou Jun 23, 2025 •

edited

Loading

dimas-b left a comment •

edited

Loading