You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "gh-yzou (via GitHub)" <gi...@apache.org> on 2025/06/18 18:21:54 UTC
[PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
gh-yzou opened a new pull request, #1908:
URL: https://github.com/apache/polaris/pull/1908
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. 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 we are using all libraries shipped along with the iceberg-spark-runtime, it simplifies the build for client significantly
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou merged PR #1908:
URL: https://github.com/apache/polaris/pull/1908
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2162435780
##########
codestyle/checkstyle.xml:
##########
@@ -26,6 +26,10 @@
<property name="severity" value="warning"/>
<property name="fileExtensions" value="java, properties, xml"/>
+ <module name="SuppressionFilter">
+ <property name="file" value="${checkstyle.suppression.file}"/>
Review Comment:
i moved the suppress file to the polaris-spark project, and reuses the original configuration
##########
build-logic/src/main/kotlin/polaris-java.gradle.kts:
##########
@@ -47,6 +47,11 @@ checkstyle {
.requiredVersion
toolVersion = checkstyleVersion
configFile = rootProject.file("codestyle/checkstyle.xml")
+ configProperties =
+ mapOf(
+ "checkstyle.suppression.file" to
+ rootProject.file("codestyle/checkstyle_suppressions.xml").absolutePath
Review Comment:
moved to project specific
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2159744142
##########
codestyle/checkstyle_suppressions.xml:
##########
@@ -0,0 +1,32 @@
+<?xml version="1.0"?>
+<!--
+ 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.
+-->
+<!DOCTYPE suppressions PUBLIC
+ "-//Puppy Crawl//DTD Suppressions Configuration 1.2//EN"
+ "https://checkstyle.org/dtds/suppressions_1_2.dtd">
+
+<suppressions>
+ <!--
+ Suppress IllegalImport in all files under plugins/spark/v3.5/spark.
+ The spark client relies on the shaded libraries from iceberg-spark-runtime,
+ and therefore uses imports like org.apache.iceberg.shaded.* in the code,
+ which is intended.
+ -->
+ <suppress checks="IllegalImport" files="plugins/spark/v3.5/spark/.*"/>
Review Comment:
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 come into 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.
##########
codestyle/checkstyle.xml:
##########
@@ -26,6 +26,10 @@
<property name="severity" value="warning"/>
<property name="fileExtensions" value="java, properties, xml"/>
+ <module name="SuppressionFilter">
+ <property name="file" value="${checkstyle.suppression.file}"/>
Review Comment:
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
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2155235838
##########
codestyle/checkstyle.xml:
##########
@@ -39,13 +39,5 @@
<property name="optional" value="true"/>
</module>
- <module name="TreeWalker">
- <!-- Checks for imports -->
- <!-- See http://checkstyle.org/config_imports.html -->
Review Comment:
@adutra Since now we actually start switching to use the shaded library, this check doesn't make sense anymore, i removed it for now.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2155235838
##########
codestyle/checkstyle.xml:
##########
@@ -39,13 +39,5 @@
<property name="optional" value="true"/>
</module>
- <module name="TreeWalker">
- <!-- Checks for imports -->
- <!-- See http://checkstyle.org/config_imports.html -->
Review Comment:
@adutra Since now we actually start switching to use the shaded library, this check doesn't make sense anymore, i removed it for now.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2159725586
##########
codestyle/checkstyle.xml:
##########
@@ -26,6 +26,10 @@
<property name="severity" value="warning"/>
<property name="fileExtensions" value="java, properties, xml"/>
+ <module name="SuppressionFilter">
+ <property name="file" value="${checkstyle.suppression.file}"/>
Review Comment:
Is this not the same as line 41?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2159744142
##########
codestyle/checkstyle_suppressions.xml:
##########
@@ -0,0 +1,32 @@
+<?xml version="1.0"?>
+<!--
+ 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.
+-->
+<!DOCTYPE suppressions PUBLIC
+ "-//Puppy Crawl//DTD Suppressions Configuration 1.2//EN"
+ "https://checkstyle.org/dtds/suppressions_1_2.dtd">
+
+<suppressions>
+ <!--
+ Suppress IllegalImport in all files under plugins/spark/v3.5/spark.
+ The spark client relies on the shaded libraries from iceberg-spark-runtime,
+ and therefore uses imports like org.apache.iceberg.shaded.* in the code,
+ which is intended.
+ -->
+ <suppress checks="IllegalImport" files="plugins/spark/v3.5/spark/.*"/>
Review Comment:
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2155243468
##########
plugins/spark/v3.5/integration/build.gradle.kts:
##########
@@ -45,9 +45,16 @@ dependencies {
implementation(project(":polaris-runtime-service"))
- testImplementation(project(":polaris-api-management-model"))
+ testImplementation(
+ "org.apache.iceberg:iceberg-spark-runtime-${sparkMajorVersion}_${scalaVersion}:${icebergVersion}"
+ )
testImplementation(project(":polaris-spark-${sparkMajorVersion}_${scalaVersion}"))
+ testImplementation(project(":polaris-api-management-model")) {
+ // exclude the iceberg
+ exclude("org.apache.iceberg", "iceberg-core")
Review Comment:
Having a dependency from `polaris-api-management-model` to `iceberg-core` is a bit concerning, IMHO, from the overall API design POV. Why should Polaris' own Management API depend on Iceberg core? This is something for follow-up discussions, of course.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on PR #1908:
URL: https://github.com/apache/polaris/pull/1908#issuecomment-2992991777
@flyrain i added the stacktrace in the PR description.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2155251817
##########
codestyle/checkstyle.xml:
##########
@@ -39,13 +39,5 @@
<property name="optional" value="true"/>
</module>
- <module name="TreeWalker">
- <!-- Checks for imports -->
- <!-- See http://checkstyle.org/config_imports.html -->
- <module name="IllegalImport">
- <property name="illegalPkgs" value=".*\.shaded\..*, .*\.relocated\..*"/>
Review Comment:
-1 to this specific change. I think an exception (cf. line 38) should be added to Spark-related modules as opposed to dropping this check for all modules.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2162442195
##########
plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/rest/GenericTable.java:
##########
@@ -0,0 +1,190 @@
+/*
+ * 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.polaris.spark.rest;
+
+import jakarta.validation.constraints.NotNull;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import org.apache.iceberg.shaded.com.fasterxml.jackson.annotation.JsonCreator;
+import org.apache.iceberg.shaded.com.fasterxml.jackson.annotation.JsonProperty;
+
+// TODO: auto generate the class based on spec
+@jakarta.annotation.Generated(
Review Comment:
i see. yes, we do not need the @Generated annotation here, removed.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #1908:
URL: https://github.com/apache/polaris/pull/1908#issuecomment-2992182086
I raised some concerns about this PR on the dev ML: https://lists.apache.org/thread/0z30f3cfvm41hxlbxgp4fqdpv7mfgnv8
Let's resolve that discussion before merging.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2159332717
##########
codestyle/checkstyle_no_illegalimport.xml:
##########
Review Comment:
I'm not sure this approach is ideal for making exceptions to Checkstyle rules.
@adutra : WDYT?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2159725586
##########
codestyle/checkstyle.xml:
##########
@@ -26,6 +26,10 @@
<property name="severity" value="warning"/>
<property name="fileExtensions" value="java, properties, xml"/>
+ <module name="SuppressionFilter">
+ <property name="file" value="${checkstyle.suppression.file}"/>
Review Comment:
Is this not the same as lines 41-42?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2162439416
##########
plugins/spark/v3.5/integration/build.gradle.kts:
##########
@@ -64,13 +68,7 @@ dependencies {
testImplementation("io.delta:delta-spark_${scalaVersion}:3.3.1")
testImplementation(platform(libs.jackson.bom))
- testImplementation("com.fasterxml.jackson.core:jackson-annotations")
- testImplementation("com.fasterxml.jackson.core:jackson-core")
- testImplementation("com.fasterxml.jackson.core:jackson-databind")
-
- testImplementation(
- "org.apache.iceberg:iceberg-spark-runtime-${sparkMajorVersion}_${scalaVersion}:${icebergVersion}"
- )
+ testImplementation("com.fasterxml.jackson.jakarta.rs:jackson-jakarta-rs-json-provider")
Review Comment:
That one is used by a polaris client test utility that is used to talk to the polaris management API for test setup, which has nothing to do with Iceberg, and not suppose to rely on the iceberg spark client library.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2159691339
##########
plugins/spark/v3.5/integration/src/intTest/java/org/apache/polaris/spark/quarkus/it/PolarisManagementClient.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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.polaris.spark.quarkus.it;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+import static org.apache.polaris.service.it.ext.PolarisServerManagerLoader.polarisServerManager;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.jakarta.rs.json.JacksonJsonProvider;
+import jakarta.ws.rs.client.Client;
+import jakarta.ws.rs.client.ClientBuilder;
+import java.util.Map;
+import java.util.Random;
+import org.apache.iceberg.rest.HTTPClient;
+import org.apache.iceberg.rest.RESTClient;
+import org.apache.iceberg.rest.auth.AuthSession;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.rest.responses.OAuthTokenResponse;
+import org.apache.polaris.service.it.env.ClientCredentials;
+import org.apache.polaris.service.it.env.ManagementApi;
+import org.apache.polaris.service.it.env.PolarisApiEndpoints;
+
+/**
+ * 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.
+ */
Review Comment:
updated
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2162434437
##########
codestyle/checkstyle_suppressions.xml:
##########
@@ -0,0 +1,32 @@
+<?xml version="1.0"?>
+<!--
+ 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.
+-->
+<!DOCTYPE suppressions PUBLIC
+ "-//Puppy Crawl//DTD Suppressions Configuration 1.2//EN"
+ "https://checkstyle.org/dtds/suppressions_1_2.dtd">
+
+<suppressions>
+ <!--
+ Suppress IllegalImport in all files under plugins/spark/v3.5/spark.
+ The spark client relies on the shaded libraries from iceberg-spark-runtime,
+ and therefore uses imports like org.apache.iceberg.shaded.* in the code,
+ which is intended.
+ -->
+ <suppress checks="IllegalImport" files="plugins/spark/v3.5/spark/.*"/>
Review Comment:
Moved the suppress config to spark client side.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2162439416
##########
plugins/spark/v3.5/integration/build.gradle.kts:
##########
@@ -64,13 +68,7 @@ dependencies {
testImplementation("io.delta:delta-spark_${scalaVersion}:3.3.1")
testImplementation(platform(libs.jackson.bom))
- testImplementation("com.fasterxml.jackson.core:jackson-annotations")
- testImplementation("com.fasterxml.jackson.core:jackson-core")
- testImplementation("com.fasterxml.jackson.core:jackson-databind")
-
- testImplementation(
- "org.apache.iceberg:iceberg-spark-runtime-${sparkMajorVersion}_${scalaVersion}:${icebergVersion}"
- )
+ testImplementation("com.fasterxml.jackson.jakarta.rs:jackson-jakarta-rs-json-provider")
Review Comment:
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2155237446
##########
plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/PolarisSparkCatalog.java:
##########
@@ -22,7 +22,7 @@
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.exceptions.AlreadyExistsException;
import org.apache.iceberg.spark.Spark3Util;
-import org.apache.polaris.service.types.GenericTable;
+import org.apache.polaris.spark.rest.GenericTable;
Review Comment:
This change is fine from my POV, but it does not seem to match the PR title and description. Please mention this refactoring there.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2159691525
##########
codestyle/checkstyle_no_illegalimport.xml:
##########
Review Comment:
switched to use suppress rule now
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2159726544
##########
codestyle/checkstyle_suppressions.xml:
##########
@@ -0,0 +1,32 @@
+<?xml version="1.0"?>
+<!--
+ 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.
+-->
+<!DOCTYPE suppressions PUBLIC
+ "-//Puppy Crawl//DTD Suppressions Configuration 1.2//EN"
+ "https://checkstyle.org/dtds/suppressions_1_2.dtd">
+
+<suppressions>
+ <!--
+ Suppress IllegalImport in all files under plugins/spark/v3.5/spark.
+ The spark client relies on the shaded libraries from iceberg-spark-runtime,
+ and therefore uses imports like org.apache.iceberg.shaded.* in the code,
+ which is intended.
+ -->
+ <suppress checks="IllegalImport" files="plugins/spark/v3.5/spark/.*"/>
Review Comment:
Is it possible to keep this file under the Spark module as opposed to the global `codestyle` dir?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #1908:
URL: https://github.com/apache/polaris/pull/1908#issuecomment-2992158105
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2159306810
##########
plugins/spark/v3.5/integration/build.gradle.kts:
##########
@@ -45,9 +45,13 @@ dependencies {
implementation(project(":polaris-runtime-service"))
- testImplementation(project(":polaris-api-management-model"))
+ testImplementation(
+ "org.apache.iceberg:iceberg-spark-runtime-${sparkMajorVersion}_${scalaVersion}:${icebergVersion}"
+ )
testImplementation(project(":polaris-spark-${sparkMajorVersion}_${scalaVersion}"))
+ testImplementation(project(":polaris-api-management-model"))
Review Comment:
+1 this provides more flexibility on the client side by minimizing dependencies, as it only depends on the Polaris REST spec now.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2159414955
##########
plugins/spark/v3.5/spark/build.gradle.kts:
##########
@@ -78,15 +57,9 @@ dependencies {
exclude("org.apache.commons", "*")
}
- implementation("org.apache.iceberg:iceberg-core:${icebergVersion}")
Review Comment:
cc @dimas-b, it doesn't depend on iceberg-core anymore, not leaking of transitive dependencies.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2155247171
##########
plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/rest/GenericTable.java:
##########
@@ -0,0 +1,190 @@
+/*
+ * 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.polaris.spark.rest;
+
+import jakarta.validation.constraints.NotNull;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import org.apache.iceberg.shaded.com.fasterxml.jackson.annotation.JsonCreator;
+import org.apache.iceberg.shaded.com.fasterxml.jackson.annotation.JsonProperty;
+
+// TODO: auto generate the class based on spec
+@jakarta.annotation.Generated(
Review Comment:
+1 to the TODO, but as of now this class is checked in to the source repo, not generated at build time :thinking:
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2159728265
##########
plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/rest/GenericTable.java:
##########
@@ -0,0 +1,190 @@
+/*
+ * 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.polaris.spark.rest;
+
+import jakarta.validation.constraints.NotNull;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import org.apache.iceberg.shaded.com.fasterxml.jackson.annotation.JsonCreator;
+import org.apache.iceberg.shaded.com.fasterxml.jackson.annotation.JsonProperty;
+
+// TODO: auto generate the class based on spec
+@jakarta.annotation.Generated(
Review Comment:
I mean: do we need the `@Generated` annotation here? The file appears to be modified after generation and manually committed to git :thinking:
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2161632699
##########
plugins/spark/v3.5/integration/build.gradle.kts:
##########
@@ -64,13 +68,7 @@ dependencies {
testImplementation("io.delta:delta-spark_${scalaVersion}:3.3.1")
testImplementation(platform(libs.jackson.bom))
- testImplementation("com.fasterxml.jackson.core:jackson-annotations")
- testImplementation("com.fasterxml.jackson.core:jackson-core")
- testImplementation("com.fasterxml.jackson.core:jackson-databind")
-
- testImplementation(
- "org.apache.iceberg:iceberg-spark-runtime-${sparkMajorVersion}_${scalaVersion}:${icebergVersion}"
- )
+ testImplementation("com.fasterxml.jackson.jakarta.rs:jackson-jakarta-rs-json-provider")
Review Comment:
Why have two Jackson in tests - the relocated one and this?
##########
codestyle/checkstyle_suppressions.xml:
##########
@@ -0,0 +1,32 @@
+<?xml version="1.0"?>
+<!--
+ 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.
+-->
+<!DOCTYPE suppressions PUBLIC
+ "-//Puppy Crawl//DTD Suppressions Configuration 1.2//EN"
+ "https://checkstyle.org/dtds/suppressions_1_2.dtd">
+
+<suppressions>
+ <!--
+ Suppress IllegalImport in all files under plugins/spark/v3.5/spark.
+ The spark client relies on the shaded libraries from iceberg-spark-runtime,
+ and therefore uses imports like org.apache.iceberg.shaded.* in the code,
+ which is intended.
+ -->
+ <suppress checks="IllegalImport" files="plugins/spark/v3.5/spark/.*"/>
Review Comment:
Agree with @dimas-b , let's not add module specifics here globally - that _does_ affect all modules
##########
build-logic/src/main/kotlin/polaris-java.gradle.kts:
##########
@@ -47,6 +47,11 @@ checkstyle {
.requiredVersion
toolVersion = checkstyleVersion
configFile = rootProject.file("codestyle/checkstyle.xml")
+ configProperties =
+ mapOf(
+ "checkstyle.suppression.file" to
+ rootProject.file("codestyle/checkstyle_suppressions.xml").absolutePath
Review Comment:
Please do not add a project specific change to all projects?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2155583603
##########
plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/PolarisSparkCatalog.java:
##########
@@ -22,7 +22,7 @@
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.exceptions.AlreadyExistsException;
import org.apache.iceberg.spark.Spark3Util;
-import org.apache.polaris.service.types.GenericTable;
+import org.apache.polaris.spark.rest.GenericTable;
Review Comment:
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.
##########
plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/PolarisSparkCatalog.java:
##########
@@ -22,7 +22,7 @@
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.exceptions.AlreadyExistsException;
import org.apache.iceberg.spark.Spark3Util;
-import org.apache.polaris.service.types.GenericTable;
+import org.apache.polaris.spark.rest.GenericTable;
Review Comment:
I added a comment here, and also updated the description to contain more details
##########
plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/rest/GenericTable.java:
##########
@@ -0,0 +1,190 @@
+/*
+ * 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.polaris.spark.rest;
+
+import jakarta.validation.constraints.NotNull;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import org.apache.iceberg.shaded.com.fasterxml.jackson.annotation.JsonCreator;
+import org.apache.iceberg.shaded.com.fasterxml.jackson.annotation.JsonProperty;
+
+// TODO: auto generate the class based on spec
+@jakarta.annotation.Generated(
Review Comment:
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 https://github.com/apache/polaris/issues/1909
##########
plugins/spark/v3.5/integration/build.gradle.kts:
##########
@@ -45,9 +45,16 @@ dependencies {
implementation(project(":polaris-runtime-service"))
- testImplementation(project(":polaris-api-management-model"))
+ testImplementation(
+ "org.apache.iceberg:iceberg-spark-runtime-${sparkMajorVersion}_${scalaVersion}:${icebergVersion}"
+ )
testImplementation(project(":polaris-spark-${sparkMajorVersion}_${scalaVersion}"))
+ testImplementation(project(":polaris-api-management-model")) {
+ // exclude the iceberg
+ exclude("org.apache.iceberg", "iceberg-core")
Review Comment:
sorry, that is actually a false alarm, i double checked the dependency graph, this project doesn't rely on iceberg-core, removed the exclude
##########
codestyle/checkstyle.xml:
##########
@@ -39,13 +39,5 @@
<property name="optional" value="true"/>
</module>
- <module name="TreeWalker">
- <!-- Checks for imports -->
- <!-- See http://checkstyle.org/config_imports.html -->
- <module name="IllegalImport">
- <property name="illegalPkgs" value=".*\.shaded\..*, .*\.relocated\..*"/>
Review Comment:
sure. I removed it because that check was originally just introduced by spark client. however it might be useful for other module also.
Since only one config file can be used, i introduces another style check file without this particular rule, and use that for the spark client 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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2159302577
##########
plugins/spark/v3.5/integration/src/intTest/java/org/apache/polaris/spark/quarkus/it/PolarisManagementClient.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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.polaris.spark.quarkus.it;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+import static org.apache.polaris.service.it.ext.PolarisServerManagerLoader.polarisServerManager;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.jakarta.rs.json.JacksonJsonProvider;
+import jakarta.ws.rs.client.Client;
+import jakarta.ws.rs.client.ClientBuilder;
+import java.util.Map;
+import java.util.Random;
+import org.apache.iceberg.rest.HTTPClient;
+import org.apache.iceberg.rest.RESTClient;
+import org.apache.iceberg.rest.auth.AuthSession;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.rest.responses.OAuthTokenResponse;
+import org.apache.polaris.service.it.env.ClientCredentials;
+import org.apache.polaris.service.it.env.ManagementApi;
+import org.apache.polaris.service.it.env.PolarisApiEndpoints;
+
+/**
+ * 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.
+ */
Review Comment:
Nit:
```suggestion
/**
* 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).
*/
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2159742321
##########
codestyle/checkstyle.xml:
##########
@@ -26,6 +26,10 @@
<property name="severity" value="warning"/>
<property name="fileExtensions" value="java, properties, xml"/>
+ <module name="SuppressionFilter">
+ <property name="file" value="${checkstyle.suppression.file}"/>
Review Comment:
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
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update spark client to use the shaded iceberg-core in iceberg-spark-runtime to avoid spark compatibilities issue [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1908:
URL: https://github.com/apache/polaris/pull/1908#discussion_r2162537700
##########
codestyle/checkstyle_suppressions.xml:
##########
@@ -0,0 +1,32 @@
+<?xml version="1.0"?>
+<!--
+ 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.
+-->
+<!DOCTYPE suppressions PUBLIC
+ "-//Puppy Crawl//DTD Suppressions Configuration 1.2//EN"
+ "https://checkstyle.org/dtds/suppressions_1_2.dtd">
+
+<suppressions>
+ <!--
+ Suppress IllegalImport in all files under plugins/spark/v3.5/spark.
+ The spark client relies on the shaded libraries from iceberg-spark-runtime,
+ and therefore uses imports like org.apache.iceberg.shaded.* in the code,
+ which is intended.
+ -->
+ <suppress checks="IllegalImport" files="plugins/spark/v3.5/spark/.*"/>
Review Comment:
thx!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org