You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "MonkeyCanCode (via GitHub)" <gi...@apache.org> on 2026/06/04 05:38:38 UTC
[PR] Spark 4 client support [polaris]
MonkeyCanCode opened a new pull request, #4612:
URL: https://github.com/apache/polaris/pull/4612
<!--
๐ Describe what changes you're proposing, especially breaking or user-facing changes.
๐ See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
This PR added spark 4 support for polaris spark client (this was previously worked on by @adam-christian-software via https://github.com/apache/polaris/pull/3188), this also fix issue https://github.com/apache/polaris/issues/3021. Beside the spark 4 support, this PR also fixes couple nits while I was working on this change such as:
1. wrong reference path to README.md
2. typo in various files
3. Minor wording changes on `plugins/spark/README.md` to make it more generic to support both spark v3.5 and v4.0
*This is rework for https://github.com/apache/polaris/pull/4535 where my accidentally messed up my branch*
## Checklist
- [x] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [x] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [x] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [x] ๐ก Added comments for complex logic
- [x] ๐งพ Updated `CHANGELOG.md` (if needed)
- [x] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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] Spark 4 client support [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3376077279
##########
plugins/spark/v3.5/spark/BUNDLE-LICENSE:
##########
@@ -204,7 +204,8 @@
This product bundles and includes code from Apache Iceberg.
-* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/BasePolarisSparkCatalog.java
* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/SparkCatalog.java
Review Comment:
+1 to a deeper follow-up review of license / deps in the Spark 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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode closed pull request #4612: Spark 4 client support
URL: https://github.com/apache/polaris/pull/4612
--
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] Spark 4 client support [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4623457059
@MonkeyCanCode : sorry, I might be out-of-date on this... why would we want to present commit history in a PR on merge?
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3368671938
##########
plugins/spark/v3.5/spark/BUNDLE-LICENSE:
##########
@@ -204,7 +204,8 @@
This product bundles and includes code from Apache Iceberg.
-* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/BasePolarisSparkCatalog.java
* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/SparkCatalog.java
Review Comment:
@flyrain I made this license change here but it may not be necessary. Should not be a blocker for the PR as we can fix this later.
--
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] Spark 4 client support [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4632905589
Re: git commit and annotations - my IJ is able to show previous authors and git log on the moved `SparkIntegrationBase` class after a squashed merge (screenshot below). Are there any other files that are not tracked properly?
<img width="3203" height="2195" alt="image" src="https://github.com/user-attachments/assets/846344e6-1d4a-4a87-83df-cf45d1a8f0ce" />
--
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] Spark 4 client support [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4648078844
> ... now we have the non-squash merge. Any last thing we should fix before merge this one?
I haven't looked at the details yet, but it appears that the integration-test and regtest issues are addressed - thanks for that!
About merging PRs, I think we should either wait for a consensus on [this mailing-list discussion](https://lists.apache.org/thread/92x2yz3ckjx31kfz77js90wyhsoxxq86) or use "Squash and merge".
--
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] Spark 4 client support [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3358057345
##########
plugins/spark/v4.0/spark/src/main/java/org/apache/polaris/spark/utils/PolarisCatalogUtils.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.utils;
+
+import com.google.common.collect.Maps;
+import java.lang.reflect.Field;
+import java.util.Map;
+import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.rest.RESTCatalog;
+import org.apache.iceberg.rest.RESTSessionCatalog;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.spark.SparkCatalog;
+import org.apache.polaris.spark.rest.GenericTable;
+import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.catalyst.TableIdentifier;
+import org.apache.spark.sql.catalyst.catalog.CatalogTable;
+import org.apache.spark.sql.catalyst.catalog.CatalogTableType;
+import org.apache.spark.sql.connector.catalog.Identifier;
+import org.apache.spark.sql.connector.catalog.Table;
+import org.apache.spark.sql.connector.catalog.TableCatalog;
+import org.apache.spark.sql.connector.catalog.TableProvider;
+import org.apache.spark.sql.execution.datasources.DataSource;
+import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Utils;
+import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import scala.Option;
+import scala.Tuple2;
+import scala.collection.immutable.Map$;
+import scala.collection.mutable.Builder;
+
+public class PolarisCatalogUtils {
+
+ public static final String TABLE_PROVIDER_KEY = "provider";
+ public static final String TABLE_PATH_KEY = "path";
+
+ /** Check whether the table provider is iceberg. */
+ public static boolean useIceberg(String provider) {
+ return provider == null || "iceberg".equalsIgnoreCase(provider);
Review Comment:
But the name does not have to be the same, I guess :thinking:
Common: PolarisCatalogUtils
Spark 3: PolarisCatalogSpark3Utils extends PolarisCatalogUtils
Spark 4: PolarisCatalogSpark4Utils extends PolarisCatalogUtils
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4643612313
@flyrain @dimas-b this PR had been rebased and now we have the non-squash merge. Any last thing we should fix before merge this one?
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4633095965
@dimas-b @jbonofre @flyrain so the history are now preserved...the catch is someone with repo edit access must enable non-squash merge before we merge this. Otherwise, the history will be gone once we click "Squash and merge".
--
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] Spark 4 client support [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4636454652
> @dimas-b @jbonofre @flyrain so the history are now preserved...the catch is someone with repo edit access must enable non-squash merge before we merge this. Otherwise, the history will be gone once we click "Squash and merge".
Thanks a lot @MonkeyCanCode! Hi @jbonofre, would you help with this?
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4622668117
Hello @snazy @dimas-b @flyrain @gh-yzou , mind kind if this what we want to do to avoid dup code? Also, it doesn't appear this repo merge without squash (setting on the repo). We will need to enable that to be able to preserve the commit histories for the spark v4.0.
Also, I didn't create regtest in for spark v4.0 as those will be replace with the JUnit directly via https://github.com/apache/polaris/pull/4588
--
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] Spark 4 client support [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4623653759
> [...] the copied files with small modifications will still show the original history on the unmodified lines.
I think git-based tools can figure line-by-line annotations across copy/move even if we do a squash merge... Please try it locally. I'm pretty sure it should be ok.
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4623470245
> @MonkeyCanCode : sorry, I might be out-of-date on this... why would we want to present commit history in a PR on merge?
By doing so, the copied files with small modifications will still show the original history on the unmodified lines.
--
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] Spark 4 client support [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4636463010
Is there any license change needed? Just wondering, we could fix license in a followup if needed.
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3377337453
##########
plugins/spark/v3.5/spark/BUNDLE-LICENSE:
##########
@@ -204,7 +204,8 @@
This product bundles and includes code from Apache Iceberg.
-* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/BasePolarisSparkCatalog.java
* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/SparkCatalog.java
Review Comment:
Tracker: https://github.com/apache/polaris/issues/4663
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3358103709
##########
plugins/spark/v4.0/spark/src/main/java/org/apache/polaris/spark/utils/PolarisCatalogUtils.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.utils;
+
+import com.google.common.collect.Maps;
+import java.lang.reflect.Field;
+import java.util.Map;
+import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.rest.RESTCatalog;
+import org.apache.iceberg.rest.RESTSessionCatalog;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.spark.SparkCatalog;
+import org.apache.polaris.spark.rest.GenericTable;
+import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.catalyst.TableIdentifier;
+import org.apache.spark.sql.catalyst.catalog.CatalogTable;
+import org.apache.spark.sql.catalyst.catalog.CatalogTableType;
+import org.apache.spark.sql.connector.catalog.Identifier;
+import org.apache.spark.sql.connector.catalog.Table;
+import org.apache.spark.sql.connector.catalog.TableCatalog;
+import org.apache.spark.sql.connector.catalog.TableProvider;
+import org.apache.spark.sql.execution.datasources.DataSource;
+import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Utils;
+import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import scala.Option;
+import scala.Tuple2;
+import scala.collection.immutable.Map$;
+import scala.collection.mutable.Builder;
+
+public class PolarisCatalogUtils {
+
+ public static final String TABLE_PROVIDER_KEY = "provider";
+ public static final String TABLE_PATH_KEY = "path";
+
+ /** Check whether the table provider is iceberg. */
+ public static boolean useIceberg(String provider) {
+ return provider == null || "iceberg".equalsIgnoreCase(provider);
Review Comment:
Yes. I like this idea. Let me do it later tonight with the remaining 4 files.
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3363662810
##########
plugins/spark/v3.5/spark/BUNDLE-LICENSE:
##########
@@ -204,7 +204,8 @@
This product bundles and includes code from Apache Iceberg.
-* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/BasePolarisSparkCatalog.java
* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/SparkCatalog.java
Review Comment:
Yes. they are no longer the same but wondering if we should keep them as those are based on that.
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode merged PR #4612:
URL: https://github.com/apache/polaris/pull/4612
--
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] Spark 4 client support [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3376077279
##########
plugins/spark/v3.5/spark/BUNDLE-LICENSE:
##########
@@ -204,7 +204,8 @@
This product bundles and includes code from Apache Iceberg.
-* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/BasePolarisSparkCatalog.java
* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/SparkCatalog.java
Review Comment:
+1 to for a deeper follow-up review of license / deps in the Spark 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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4655253268
As there is no concern with non-squash merge from ML, I will merge this PR now. We still have following follow up PRs open for this topics:
1. spark 4 regression test: https://github.com/apache/polaris/issues/4664
2. spark 4 license review: https://github.com/apache/polaris/issues/4663
--
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] Spark 4 client support [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3368772868
##########
plugins/spark/v3.5/spark/BUNDLE-LICENSE:
##########
@@ -204,7 +204,8 @@
This product bundles and includes code from Apache Iceberg.
-* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/BasePolarisSparkCatalog.java
* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/SparkCatalog.java
Review Comment:
not a blocker at all. Just want to make sure we don't forget the followup, as we have to do it anyway for the next release.
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4624797095
> > [...] the copied files with small modifications will still show the original history on the unmodified lines.
>
> I think git-based tools can figure line-by-line annotations across copy/move even if we do a squash merge... Please try it locally. I'm pretty sure it should be ok.
So we did that already. With plain `cp src dest` in original PR, this will all show the works are done by me. This is requested via https://github.com/apache/polaris/pull/4535#issuecomment-4560086468 which iceberg team had to do the same as well.
--
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] Spark 4 client support [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3363579578
##########
plugins/spark/v3.5/spark/BUNDLE-LICENSE:
##########
@@ -204,7 +204,8 @@
This product bundles and includes code from Apache Iceberg.
-* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/BasePolarisSparkCatalog.java
* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/SparkCatalog.java
Review Comment:
Are these classes really "copied from Iceberg" anymore? My reading is that they are very different 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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4628128484
@dimas-b mind take another look?
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4633102510
> Re: git commit and annotations - my IJ is able to show previous authors and git log on the moved `SparkIntegrationBase` class after a squashed merge (screenshot below). Are there any other files that are not tracked properly?
>
> <img alt="image" width="2000" height="1371" src="https://private-user-images.githubusercontent.com/40603221/603529596-846344e6-1d4a-4a87-83df-cf45d1a8f0ce.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3ODA2NzM1NjQsIm5iZiI6MTc4MDY3MzI2NCwicGF0aCI6Ii80MDYwMzIyMS82MDM1Mjk1OTYtODQ2MzQ0ZTYtMWQ0YS00YTg3LTgzZGYtY2Y0NWQxYThmMGNlLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNjA2MDUlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjYwNjA1VDE1Mjc0NFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTg1MmMzM2YwYzUwM2E1ODdlMDRlZDJjZTc5Y2EyMmQ0N2U3MWM3OTRjNmRlZGE4OTFlZjZkZjBmZGIxZmYxYzMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JnJlc3BvbnNlLWNvbnRlbnQtdHlwZT1pbWFnZSUyRnBuZyJ9.DluN4gLJEulGxpnteMzYpfwqVT5WC5U7ZWaSlmMWzao">
So all files (including the new base interface) should have previous history with this approach. This also means, the spark 4 files will have history even they are modification on top of spark 3.
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3358103709
##########
plugins/spark/v4.0/spark/src/main/java/org/apache/polaris/spark/utils/PolarisCatalogUtils.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.utils;
+
+import com.google.common.collect.Maps;
+import java.lang.reflect.Field;
+import java.util.Map;
+import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.rest.RESTCatalog;
+import org.apache.iceberg.rest.RESTSessionCatalog;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.spark.SparkCatalog;
+import org.apache.polaris.spark.rest.GenericTable;
+import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.catalyst.TableIdentifier;
+import org.apache.spark.sql.catalyst.catalog.CatalogTable;
+import org.apache.spark.sql.catalyst.catalog.CatalogTableType;
+import org.apache.spark.sql.connector.catalog.Identifier;
+import org.apache.spark.sql.connector.catalog.Table;
+import org.apache.spark.sql.connector.catalog.TableCatalog;
+import org.apache.spark.sql.connector.catalog.TableProvider;
+import org.apache.spark.sql.execution.datasources.DataSource;
+import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Utils;
+import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import scala.Option;
+import scala.Tuple2;
+import scala.collection.immutable.Map$;
+import scala.collection.mutable.Builder;
+
+public class PolarisCatalogUtils {
+
+ public static final String TABLE_PROVIDER_KEY = "provider";
+ public static final String TABLE_PATH_KEY = "path";
+
+ /** Check whether the table provider is iceberg. */
+ public static boolean useIceberg(String provider) {
+ return provider == null || "iceberg".equalsIgnoreCase(provider);
Review Comment:
Yes. I live this idea. Let me do it later tonight with the remaining 4 files.
--
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] Spark 4 client support [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3357084036
##########
plugins/spark/v4.0/spark/src/main/java/org/apache/polaris/spark/utils/PolarisCatalogUtils.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.utils;
+
+import com.google.common.collect.Maps;
+import java.lang.reflect.Field;
+import java.util.Map;
+import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.rest.RESTCatalog;
+import org.apache.iceberg.rest.RESTSessionCatalog;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.spark.SparkCatalog;
+import org.apache.polaris.spark.rest.GenericTable;
+import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.catalyst.TableIdentifier;
+import org.apache.spark.sql.catalyst.catalog.CatalogTable;
+import org.apache.spark.sql.catalyst.catalog.CatalogTableType;
+import org.apache.spark.sql.connector.catalog.Identifier;
+import org.apache.spark.sql.connector.catalog.Table;
+import org.apache.spark.sql.connector.catalog.TableCatalog;
+import org.apache.spark.sql.connector.catalog.TableProvider;
+import org.apache.spark.sql.execution.datasources.DataSource;
+import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Utils;
+import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import scala.Option;
+import scala.Tuple2;
+import scala.collection.immutable.Map$;
+import scala.collection.mutable.Builder;
+
+public class PolarisCatalogUtils {
+
+ public static final String TABLE_PROVIDER_KEY = "provider";
+ public static final String TABLE_PATH_KEY = "path";
+
+ /** Check whether the table provider is iceberg. */
+ public static boolean useIceberg(String provider) {
+ return provider == null || "iceberg".equalsIgnoreCase(provider);
Review Comment:
This logic is identical in Spark 3.5 code... Could we share this code too?
##########
plugins/spark/v4.0/spark/src/main/java/org/apache/polaris/spark/utils/PolarisCatalogUtils.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.utils;
+
+import com.google.common.collect.Maps;
+import java.lang.reflect.Field;
+import java.util.Map;
+import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.rest.RESTCatalog;
+import org.apache.iceberg.rest.RESTSessionCatalog;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.spark.SparkCatalog;
+import org.apache.polaris.spark.rest.GenericTable;
+import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.catalyst.TableIdentifier;
+import org.apache.spark.sql.catalyst.catalog.CatalogTable;
+import org.apache.spark.sql.catalyst.catalog.CatalogTableType;
+import org.apache.spark.sql.connector.catalog.Identifier;
+import org.apache.spark.sql.connector.catalog.Table;
+import org.apache.spark.sql.connector.catalog.TableCatalog;
+import org.apache.spark.sql.connector.catalog.TableProvider;
+import org.apache.spark.sql.execution.datasources.DataSource;
+import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Utils;
+import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import scala.Option;
+import scala.Tuple2;
+import scala.collection.immutable.Map$;
+import scala.collection.mutable.Builder;
+
+public class PolarisCatalogUtils {
+
+ public static final String TABLE_PROVIDER_KEY = "provider";
+ public static final String TABLE_PATH_KEY = "path";
+
+ /** Check whether the table provider is iceberg. */
+ public static boolean useIceberg(String provider) {
+ return provider == null || "iceberg".equalsIgnoreCase(provider);
+ }
+
+ /** Check whether the table provider is delta. */
+ public static boolean useDelta(String provider) {
+ return "delta".equalsIgnoreCase(provider);
+ }
+
+ public static boolean useHudi(String provider) {
+ return "hudi".equalsIgnoreCase(provider);
+ }
+
+ /** Check whether the table provider is paimon. */
+ public static boolean usePaimon(String provider) {
+ return "paimon".equalsIgnoreCase(provider);
+ }
+
+ /**
+ * For tables whose location is managed by Spark Session Catalog, there will be no location or
+ * path in the properties.
+ */
+ public static boolean isTableWithSparkManagedLocation(Map<String, String> properties) {
+ boolean hasLocationClause = properties.containsKey(TableCatalog.PROP_LOCATION);
+ boolean hasPathClause = properties.containsKey(TABLE_PATH_KEY);
+ return !hasLocationClause && !hasPathClause;
+ }
+
+ /**
+ * Normalize table properties for loading Spark tables by ensuring the TABLE_PATH_KEY is properly
+ * set. DataSourceV2 requires the path property on table loading.
+ */
+ private static Map<String, String> normalizeTablePropertiesForLoadSparkTable(
+ GenericTable genericTable) {
+ Map<String, String> properties = genericTable.properties();
+ boolean hasLocationClause = properties.get(TableCatalog.PROP_LOCATION) != null;
+ boolean hasPathClause = properties.get(TABLE_PATH_KEY) != null;
+ Map<String, String> tableProperties = Maps.newHashMap();
+ tableProperties.putAll(properties);
+ if (!hasPathClause) {
+ // DataSourceV2 requires the path property on table loading. However, spark today
+ // doesn't create the corresponding path property if the path keyword is not
+ // provided by user when location is provided. Here, we duplicate the location
+ // property as path to make sure the table can be loaded.
+ if (genericTable.baseLocation() != null && !genericTable.baseLocation().isEmpty()) {
+ tableProperties.put(TABLE_PATH_KEY, genericTable.baseLocation());
+ } else if (hasLocationClause) {
+ tableProperties.put(TABLE_PATH_KEY, properties.get(TableCatalog.PROP_LOCATION));
+ }
+ }
+ return tableProperties;
+ }
+
+ /**
+ * Load spark table using DataSourceV2.
+ *
+ * @return V2Table if DataSourceV2 is available for the table format. For delta table, it returns
+ * DeltaTableV2.
+ */
+ public static Table loadV2SparkTable(GenericTable genericTable) {
+ SparkSession sparkSession = SparkSession.active();
+ TableProvider provider =
+ DataSource.lookupDataSourceV2(genericTable.format(), sparkSession.sessionState().conf())
+ .get();
+ Map<String, String> tableProperties = normalizeTablePropertiesForLoadSparkTable(genericTable);
+ return DataSourceV2Utils.getTableFromProvider(
+ provider, new CaseInsensitiveStringMap(tableProperties), scala.Option.empty());
+ }
+
+ /**
+ * Return a Spark V1Table for formats that do not use DataSourceV2. Currently, this is being used
+ * for Hudi tables
+ */
+ public static Table loadV1SparkTable(
+ GenericTable genericTable, Identifier identifier, String catalogName) {
+ Map<String, String> tableProperties = normalizeTablePropertiesForLoadSparkTable(genericTable);
+
+ // Need full identifier in order to construct CatalogTable
+ String namespacePath = String.join(".", identifier.namespace());
+ TableIdentifier tableIdentifier =
+ new TableIdentifier(
+ identifier.name(), Option.apply(namespacePath), Option.apply(catalogName));
+
+ Builder<Tuple2<String, String>, scala.collection.immutable.Map<String, String>> mb =
+ Map$.MODULE$.newBuilder();
+ tableProperties.forEach((k, v) -> mb.$plus$eq(Tuple2.apply(k, v)));
+ scala.collection.immutable.Map<String, String> scalaOptions = mb.result();
+
+ org.apache.spark.sql.catalyst.catalog.CatalogStorageFormat storage =
+ DataSource.buildStorageFormatFromOptions(scalaOptions);
+
+ // Currently Polaris generic table does not contain any schema information, partition columns,
+ // stats, etc
+ // for now we will fill the parameters we have from polaris catalog, and let underlying client
+ // resolve the rest within its catalog implementation
+ org.apache.spark.sql.types.StructType emptySchema = new org.apache.spark.sql.types.StructType();
+ @SuppressWarnings("deprecation")
+ scala.collection.immutable.Seq<String> emptyStringSeq =
+ scala.collection.JavaConverters.asScalaBuffer(new java.util.ArrayList<String>()).toList();
+ CatalogTable catalogTable =
+ new CatalogTable(
+ tableIdentifier,
+ CatalogTableType.EXTERNAL(),
+ storage,
+ emptySchema,
+ Option.apply(genericTable.format()),
+ emptyStringSeq,
+ scala.Option.empty(),
+ genericTable.properties().getOrDefault(TableCatalog.PROP_OWNER, ""),
Review Comment:
Only the default changes in Spark 4.0 code ... Could it be safe to use the default with Spark 3.5 (and have 1:1 util code)?
##########
plugins/spark/v4.0/spark/src/main/java/org/apache/polaris/spark/utils/PolarisCatalogUtils.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.utils;
+
+import com.google.common.collect.Maps;
+import java.lang.reflect.Field;
+import java.util.Map;
+import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.rest.RESTCatalog;
+import org.apache.iceberg.rest.RESTSessionCatalog;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.spark.SparkCatalog;
+import org.apache.polaris.spark.rest.GenericTable;
+import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.catalyst.TableIdentifier;
+import org.apache.spark.sql.catalyst.catalog.CatalogTable;
+import org.apache.spark.sql.catalyst.catalog.CatalogTableType;
+import org.apache.spark.sql.connector.catalog.Identifier;
+import org.apache.spark.sql.connector.catalog.Table;
+import org.apache.spark.sql.connector.catalog.TableCatalog;
+import org.apache.spark.sql.connector.catalog.TableProvider;
+import org.apache.spark.sql.execution.datasources.DataSource;
+import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Utils;
+import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import scala.Option;
+import scala.Tuple2;
+import scala.collection.immutable.Map$;
+import scala.collection.mutable.Builder;
+
+public class PolarisCatalogUtils {
+
+ public static final String TABLE_PROVIDER_KEY = "provider";
+ public static final String TABLE_PATH_KEY = "path";
+
+ /** Check whether the table provider is iceberg. */
+ public static boolean useIceberg(String provider) {
+ return provider == null || "iceberg".equalsIgnoreCase(provider);
+ }
+
+ /** Check whether the table provider is delta. */
+ public static boolean useDelta(String provider) {
+ return "delta".equalsIgnoreCase(provider);
+ }
+
+ public static boolean useHudi(String provider) {
+ return "hudi".equalsIgnoreCase(provider);
+ }
+
+ /** Check whether the table provider is paimon. */
+ public static boolean usePaimon(String provider) {
+ return "paimon".equalsIgnoreCase(provider);
+ }
+
+ /**
+ * For tables whose location is managed by Spark Session Catalog, there will be no location or
+ * path in the properties.
+ */
+ public static boolean isTableWithSparkManagedLocation(Map<String, String> properties) {
+ boolean hasLocationClause = properties.containsKey(TableCatalog.PROP_LOCATION);
+ boolean hasPathClause = properties.containsKey(TABLE_PATH_KEY);
+ return !hasLocationClause && !hasPathClause;
+ }
+
+ /**
+ * Normalize table properties for loading Spark tables by ensuring the TABLE_PATH_KEY is properly
+ * set. DataSourceV2 requires the path property on table loading.
+ */
+ private static Map<String, String> normalizeTablePropertiesForLoadSparkTable(
+ GenericTable genericTable) {
+ Map<String, String> properties = genericTable.properties();
+ boolean hasLocationClause = properties.get(TableCatalog.PROP_LOCATION) != null;
+ boolean hasPathClause = properties.get(TABLE_PATH_KEY) != null;
+ Map<String, String> tableProperties = Maps.newHashMap();
+ tableProperties.putAll(properties);
+ if (!hasPathClause) {
+ // DataSourceV2 requires the path property on table loading. However, spark today
+ // doesn't create the corresponding path property if the path keyword is not
+ // provided by user when location is provided. Here, we duplicate the location
+ // property as path to make sure the table can be loaded.
+ if (genericTable.baseLocation() != null && !genericTable.baseLocation().isEmpty()) {
+ tableProperties.put(TABLE_PATH_KEY, genericTable.baseLocation());
+ } else if (hasLocationClause) {
+ tableProperties.put(TABLE_PATH_KEY, properties.get(TableCatalog.PROP_LOCATION));
+ }
+ }
+ return tableProperties;
+ }
+
+ /**
+ * Load spark table using DataSourceV2.
+ *
+ * @return V2Table if DataSourceV2 is available for the table format. For delta table, it returns
+ * DeltaTableV2.
+ */
+ public static Table loadV2SparkTable(GenericTable genericTable) {
+ SparkSession sparkSession = SparkSession.active();
+ TableProvider provider =
+ DataSource.lookupDataSourceV2(genericTable.format(), sparkSession.sessionState().conf())
+ .get();
+ Map<String, String> tableProperties = normalizeTablePropertiesForLoadSparkTable(genericTable);
+ return DataSourceV2Utils.getTableFromProvider(
+ provider, new CaseInsensitiveStringMap(tableProperties), scala.Option.empty());
+ }
+
+ /**
+ * Return a Spark V1Table for formats that do not use DataSourceV2. Currently, this is being used
+ * for Hudi tables
+ */
+ public static Table loadV1SparkTable(
+ GenericTable genericTable, Identifier identifier, String catalogName) {
+ Map<String, String> tableProperties = normalizeTablePropertiesForLoadSparkTable(genericTable);
+
+ // Need full identifier in order to construct CatalogTable
+ String namespacePath = String.join(".", identifier.namespace());
+ TableIdentifier tableIdentifier =
+ new TableIdentifier(
+ identifier.name(), Option.apply(namespacePath), Option.apply(catalogName));
+
+ Builder<Tuple2<String, String>, scala.collection.immutable.Map<String, String>> mb =
+ Map$.MODULE$.newBuilder();
+ tableProperties.forEach((k, v) -> mb.$plus$eq(Tuple2.apply(k, v)));
+ scala.collection.immutable.Map<String, String> scalaOptions = mb.result();
+
+ org.apache.spark.sql.catalyst.catalog.CatalogStorageFormat storage =
+ DataSource.buildStorageFormatFromOptions(scalaOptions);
+
+ // Currently Polaris generic table does not contain any schema information, partition columns,
+ // stats, etc
+ // for now we will fill the parameters we have from polaris catalog, and let underlying client
+ // resolve the rest within its catalog implementation
+ org.apache.spark.sql.types.StructType emptySchema = new org.apache.spark.sql.types.StructType();
+ @SuppressWarnings("deprecation")
+ scala.collection.immutable.Seq<String> emptyStringSeq =
+ scala.collection.JavaConverters.asScalaBuffer(new java.util.ArrayList<String>()).toList();
+ CatalogTable catalogTable =
+ new CatalogTable(
+ tableIdentifier,
+ CatalogTableType.EXTERNAL(),
+ storage,
+ emptySchema,
+ Option.apply(genericTable.format()),
+ emptyStringSeq,
+ scala.Option.empty(),
+ genericTable.properties().getOrDefault(TableCatalog.PROP_OWNER, ""),
+ System.currentTimeMillis(),
+ -1L,
+ "",
+ scalaOptions,
+ scala.Option.empty(),
Review Comment:
I may be missing some hidden issue, please take my suggestions with a grain of salt :)
##########
plugins/spark/v4.0/spark/src/main/java/org/apache/polaris/spark/utils/PolarisCatalogUtils.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.utils;
+
+import com.google.common.collect.Maps;
+import java.lang.reflect.Field;
+import java.util.Map;
+import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.rest.RESTCatalog;
+import org.apache.iceberg.rest.RESTSessionCatalog;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.spark.SparkCatalog;
+import org.apache.polaris.spark.rest.GenericTable;
+import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.catalyst.TableIdentifier;
+import org.apache.spark.sql.catalyst.catalog.CatalogTable;
+import org.apache.spark.sql.catalyst.catalog.CatalogTableType;
+import org.apache.spark.sql.connector.catalog.Identifier;
+import org.apache.spark.sql.connector.catalog.Table;
+import org.apache.spark.sql.connector.catalog.TableCatalog;
+import org.apache.spark.sql.connector.catalog.TableProvider;
+import org.apache.spark.sql.execution.datasources.DataSource;
+import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Utils;
+import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import scala.Option;
+import scala.Tuple2;
+import scala.collection.immutable.Map$;
+import scala.collection.mutable.Builder;
+
+public class PolarisCatalogUtils {
+
+ public static final String TABLE_PROVIDER_KEY = "provider";
+ public static final String TABLE_PATH_KEY = "path";
+
+ /** Check whether the table provider is iceberg. */
+ public static boolean useIceberg(String provider) {
+ return provider == null || "iceberg".equalsIgnoreCase(provider);
+ }
+
+ /** Check whether the table provider is delta. */
+ public static boolean useDelta(String provider) {
+ return "delta".equalsIgnoreCase(provider);
+ }
+
+ public static boolean useHudi(String provider) {
+ return "hudi".equalsIgnoreCase(provider);
+ }
+
+ /** Check whether the table provider is paimon. */
+ public static boolean usePaimon(String provider) {
+ return "paimon".equalsIgnoreCase(provider);
+ }
+
+ /**
+ * For tables whose location is managed by Spark Session Catalog, there will be no location or
+ * path in the properties.
+ */
+ public static boolean isTableWithSparkManagedLocation(Map<String, String> properties) {
+ boolean hasLocationClause = properties.containsKey(TableCatalog.PROP_LOCATION);
+ boolean hasPathClause = properties.containsKey(TABLE_PATH_KEY);
+ return !hasLocationClause && !hasPathClause;
+ }
+
+ /**
+ * Normalize table properties for loading Spark tables by ensuring the TABLE_PATH_KEY is properly
+ * set. DataSourceV2 requires the path property on table loading.
+ */
+ private static Map<String, String> normalizeTablePropertiesForLoadSparkTable(
+ GenericTable genericTable) {
+ Map<String, String> properties = genericTable.properties();
+ boolean hasLocationClause = properties.get(TableCatalog.PROP_LOCATION) != null;
+ boolean hasPathClause = properties.get(TABLE_PATH_KEY) != null;
+ Map<String, String> tableProperties = Maps.newHashMap();
+ tableProperties.putAll(properties);
+ if (!hasPathClause) {
+ // DataSourceV2 requires the path property on table loading. However, spark today
+ // doesn't create the corresponding path property if the path keyword is not
+ // provided by user when location is provided. Here, we duplicate the location
+ // property as path to make sure the table can be loaded.
+ if (genericTable.baseLocation() != null && !genericTable.baseLocation().isEmpty()) {
+ tableProperties.put(TABLE_PATH_KEY, genericTable.baseLocation());
+ } else if (hasLocationClause) {
+ tableProperties.put(TABLE_PATH_KEY, properties.get(TableCatalog.PROP_LOCATION));
+ }
+ }
+ return tableProperties;
+ }
+
+ /**
+ * Load spark table using DataSourceV2.
+ *
+ * @return V2Table if DataSourceV2 is available for the table format. For delta table, it returns
+ * DeltaTableV2.
+ */
+ public static Table loadV2SparkTable(GenericTable genericTable) {
+ SparkSession sparkSession = SparkSession.active();
+ TableProvider provider =
+ DataSource.lookupDataSourceV2(genericTable.format(), sparkSession.sessionState().conf())
+ .get();
+ Map<String, String> tableProperties = normalizeTablePropertiesForLoadSparkTable(genericTable);
+ return DataSourceV2Utils.getTableFromProvider(
+ provider, new CaseInsensitiveStringMap(tableProperties), scala.Option.empty());
+ }
+
+ /**
+ * Return a Spark V1Table for formats that do not use DataSourceV2. Currently, this is being used
+ * for Hudi tables
+ */
+ public static Table loadV1SparkTable(
+ GenericTable genericTable, Identifier identifier, String catalogName) {
+ Map<String, String> tableProperties = normalizeTablePropertiesForLoadSparkTable(genericTable);
+
+ // Need full identifier in order to construct CatalogTable
+ String namespacePath = String.join(".", identifier.namespace());
+ TableIdentifier tableIdentifier =
+ new TableIdentifier(
+ identifier.name(), Option.apply(namespacePath), Option.apply(catalogName));
+
+ Builder<Tuple2<String, String>, scala.collection.immutable.Map<String, String>> mb =
+ Map$.MODULE$.newBuilder();
+ tableProperties.forEach((k, v) -> mb.$plus$eq(Tuple2.apply(k, v)));
+ scala.collection.immutable.Map<String, String> scalaOptions = mb.result();
+
+ org.apache.spark.sql.catalyst.catalog.CatalogStorageFormat storage =
+ DataSource.buildStorageFormatFromOptions(scalaOptions);
+
+ // Currently Polaris generic table does not contain any schema information, partition columns,
+ // stats, etc
+ // for now we will fill the parameters we have from polaris catalog, and let underlying client
+ // resolve the rest within its catalog implementation
+ org.apache.spark.sql.types.StructType emptySchema = new org.apache.spark.sql.types.StructType();
+ @SuppressWarnings("deprecation")
+ scala.collection.immutable.Seq<String> emptyStringSeq =
+ scala.collection.JavaConverters.asScalaBuffer(new java.util.ArrayList<String>()).toList();
+ CatalogTable catalogTable =
+ new CatalogTable(
+ tableIdentifier,
+ CatalogTableType.EXTERNAL(),
+ storage,
+ emptySchema,
+ Option.apply(genericTable.format()),
+ emptyStringSeq,
+ scala.Option.empty(),
+ genericTable.properties().getOrDefault(TableCatalog.PROP_OWNER, ""),
+ System.currentTimeMillis(),
+ -1L,
+ "",
+ scalaOptions,
+ scala.Option.empty(),
Review Comment:
Only this parameter is new in Spark 4.0... How about we keep most of the code in common (base class) and just have an overridable method for specializing `new CatalogTable(...)` in Spark 3 vs 4?
##########
plugins/spark/v4.0/spark/src/main/java/org/apache/polaris/spark/utils/PolarisCatalogUtils.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.utils;
+
+import com.google.common.collect.Maps;
+import java.lang.reflect.Field;
+import java.util.Map;
+import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.rest.RESTCatalog;
+import org.apache.iceberg.rest.RESTSessionCatalog;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.spark.SparkCatalog;
+import org.apache.polaris.spark.rest.GenericTable;
+import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.catalyst.TableIdentifier;
+import org.apache.spark.sql.catalyst.catalog.CatalogTable;
+import org.apache.spark.sql.catalyst.catalog.CatalogTableType;
+import org.apache.spark.sql.connector.catalog.Identifier;
+import org.apache.spark.sql.connector.catalog.Table;
+import org.apache.spark.sql.connector.catalog.TableCatalog;
+import org.apache.spark.sql.connector.catalog.TableProvider;
+import org.apache.spark.sql.execution.datasources.DataSource;
+import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Utils;
+import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import scala.Option;
+import scala.Tuple2;
+import scala.collection.immutable.Map$;
+import scala.collection.mutable.Builder;
+
+public class PolarisCatalogUtils {
+
+ public static final String TABLE_PROVIDER_KEY = "provider";
+ public static final String TABLE_PATH_KEY = "path";
+
+ /** Check whether the table provider is iceberg. */
+ public static boolean useIceberg(String provider) {
+ return provider == null || "iceberg".equalsIgnoreCase(provider);
+ }
+
+ /** Check whether the table provider is delta. */
+ public static boolean useDelta(String provider) {
+ return "delta".equalsIgnoreCase(provider);
+ }
+
+ public static boolean useHudi(String provider) {
+ return "hudi".equalsIgnoreCase(provider);
+ }
+
+ /** Check whether the table provider is paimon. */
+ public static boolean usePaimon(String provider) {
+ return "paimon".equalsIgnoreCase(provider);
+ }
+
+ /**
+ * For tables whose location is managed by Spark Session Catalog, there will be no location or
+ * path in the properties.
+ */
+ public static boolean isTableWithSparkManagedLocation(Map<String, String> properties) {
+ boolean hasLocationClause = properties.containsKey(TableCatalog.PROP_LOCATION);
+ boolean hasPathClause = properties.containsKey(TABLE_PATH_KEY);
+ return !hasLocationClause && !hasPathClause;
+ }
+
+ /**
+ * Normalize table properties for loading Spark tables by ensuring the TABLE_PATH_KEY is properly
+ * set. DataSourceV2 requires the path property on table loading.
+ */
+ private static Map<String, String> normalizeTablePropertiesForLoadSparkTable(
+ GenericTable genericTable) {
+ Map<String, String> properties = genericTable.properties();
+ boolean hasLocationClause = properties.get(TableCatalog.PROP_LOCATION) != null;
+ boolean hasPathClause = properties.get(TABLE_PATH_KEY) != null;
+ Map<String, String> tableProperties = Maps.newHashMap();
+ tableProperties.putAll(properties);
+ if (!hasPathClause) {
+ // DataSourceV2 requires the path property on table loading. However, spark today
+ // doesn't create the corresponding path property if the path keyword is not
+ // provided by user when location is provided. Here, we duplicate the location
+ // property as path to make sure the table can be loaded.
+ if (genericTable.baseLocation() != null && !genericTable.baseLocation().isEmpty()) {
+ tableProperties.put(TABLE_PATH_KEY, genericTable.baseLocation());
+ } else if (hasLocationClause) {
+ tableProperties.put(TABLE_PATH_KEY, properties.get(TableCatalog.PROP_LOCATION));
+ }
+ }
+ return tableProperties;
+ }
+
+ /**
+ * Load spark table using DataSourceV2.
+ *
+ * @return V2Table if DataSourceV2 is available for the table format. For delta table, it returns
+ * DeltaTableV2.
+ */
+ public static Table loadV2SparkTable(GenericTable genericTable) {
+ SparkSession sparkSession = SparkSession.active();
+ TableProvider provider =
+ DataSource.lookupDataSourceV2(genericTable.format(), sparkSession.sessionState().conf())
+ .get();
+ Map<String, String> tableProperties = normalizeTablePropertiesForLoadSparkTable(genericTable);
+ return DataSourceV2Utils.getTableFromProvider(
+ provider, new CaseInsensitiveStringMap(tableProperties), scala.Option.empty());
+ }
+
+ /**
+ * Return a Spark V1Table for formats that do not use DataSourceV2. Currently, this is being used
+ * for Hudi tables
+ */
+ public static Table loadV1SparkTable(
+ GenericTable genericTable, Identifier identifier, String catalogName) {
+ Map<String, String> tableProperties = normalizeTablePropertiesForLoadSparkTable(genericTable);
+
+ // Need full identifier in order to construct CatalogTable
+ String namespacePath = String.join(".", identifier.namespace());
+ TableIdentifier tableIdentifier =
+ new TableIdentifier(
+ identifier.name(), Option.apply(namespacePath), Option.apply(catalogName));
+
+ Builder<Tuple2<String, String>, scala.collection.immutable.Map<String, String>> mb =
+ Map$.MODULE$.newBuilder();
+ tableProperties.forEach((k, v) -> mb.$plus$eq(Tuple2.apply(k, v)));
+ scala.collection.immutable.Map<String, String> scalaOptions = mb.result();
+
+ org.apache.spark.sql.catalyst.catalog.CatalogStorageFormat storage =
+ DataSource.buildStorageFormatFromOptions(scalaOptions);
+
+ // Currently Polaris generic table does not contain any schema information, partition columns,
+ // stats, etc
+ // for now we will fill the parameters we have from polaris catalog, and let underlying client
+ // resolve the rest within its catalog implementation
+ org.apache.spark.sql.types.StructType emptySchema = new org.apache.spark.sql.types.StructType();
+ @SuppressWarnings("deprecation")
+ scala.collection.immutable.Seq<String> emptyStringSeq =
+ scala.collection.JavaConverters.asScalaBuffer(new java.util.ArrayList<String>()).toList();
+ CatalogTable catalogTable =
+ new CatalogTable(
+ tableIdentifier,
+ CatalogTableType.EXTERNAL(),
+ storage,
+ emptySchema,
+ Option.apply(genericTable.format()),
+ emptyStringSeq,
+ scala.Option.empty(),
+ genericTable.properties().getOrDefault(TableCatalog.PROP_OWNER, ""),
+ System.currentTimeMillis(),
+ -1L,
+ "",
+ scalaOptions,
+ scala.Option.empty(),
Review Comment:
The apply the same principle to `SparkCatalog`?
--
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] Spark 4 client support [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3368772149
##########
plugins/spark/v3.5/spark/BUNDLE-LICENSE:
##########
@@ -204,7 +204,8 @@
This product bundles and includes code from Apache Iceberg.
-* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/BasePolarisSparkCatalog.java
* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/SparkCatalog.java
Review Comment:
My question is more about the plugins/spark/v4.0/spark/BUNDLE-LICENSE and plugins/spark/v4.0/spark/BUNDLE-NOTICE. I assume it may need certain due to transitive deps brought by Spark 4.0. Are they the same as 3.5 ones or this PR has made the change already?
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3357988240
##########
plugins/spark/v4.0/spark/src/main/java/org/apache/polaris/spark/utils/PolarisCatalogUtils.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.utils;
+
+import com.google.common.collect.Maps;
+import java.lang.reflect.Field;
+import java.util.Map;
+import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.rest.RESTCatalog;
+import org.apache.iceberg.rest.RESTSessionCatalog;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.spark.SparkCatalog;
+import org.apache.polaris.spark.rest.GenericTable;
+import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.catalyst.TableIdentifier;
+import org.apache.spark.sql.catalyst.catalog.CatalogTable;
+import org.apache.spark.sql.catalyst.catalog.CatalogTableType;
+import org.apache.spark.sql.connector.catalog.Identifier;
+import org.apache.spark.sql.connector.catalog.Table;
+import org.apache.spark.sql.connector.catalog.TableCatalog;
+import org.apache.spark.sql.connector.catalog.TableProvider;
+import org.apache.spark.sql.execution.datasources.DataSource;
+import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Utils;
+import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import scala.Option;
+import scala.Tuple2;
+import scala.collection.immutable.Map$;
+import scala.collection.mutable.Builder;
+
+public class PolarisCatalogUtils {
+
+ public static final String TABLE_PROVIDER_KEY = "provider";
+ public static final String TABLE_PATH_KEY = "path";
+
+ /** Check whether the table provider is iceberg. */
+ public static boolean useIceberg(String provider) {
+ return provider == null || "iceberg".equalsIgnoreCase(provider);
+ }
+
+ /** Check whether the table provider is delta. */
+ public static boolean useDelta(String provider) {
+ return "delta".equalsIgnoreCase(provider);
+ }
+
+ public static boolean useHudi(String provider) {
+ return "hudi".equalsIgnoreCase(provider);
+ }
+
+ /** Check whether the table provider is paimon. */
+ public static boolean usePaimon(String provider) {
+ return "paimon".equalsIgnoreCase(provider);
+ }
+
+ /**
+ * For tables whose location is managed by Spark Session Catalog, there will be no location or
+ * path in the properties.
+ */
+ public static boolean isTableWithSparkManagedLocation(Map<String, String> properties) {
+ boolean hasLocationClause = properties.containsKey(TableCatalog.PROP_LOCATION);
+ boolean hasPathClause = properties.containsKey(TABLE_PATH_KEY);
+ return !hasLocationClause && !hasPathClause;
+ }
+
+ /**
+ * Normalize table properties for loading Spark tables by ensuring the TABLE_PATH_KEY is properly
+ * set. DataSourceV2 requires the path property on table loading.
+ */
+ private static Map<String, String> normalizeTablePropertiesForLoadSparkTable(
+ GenericTable genericTable) {
+ Map<String, String> properties = genericTable.properties();
+ boolean hasLocationClause = properties.get(TableCatalog.PROP_LOCATION) != null;
+ boolean hasPathClause = properties.get(TABLE_PATH_KEY) != null;
+ Map<String, String> tableProperties = Maps.newHashMap();
+ tableProperties.putAll(properties);
+ if (!hasPathClause) {
+ // DataSourceV2 requires the path property on table loading. However, spark today
+ // doesn't create the corresponding path property if the path keyword is not
+ // provided by user when location is provided. Here, we duplicate the location
+ // property as path to make sure the table can be loaded.
+ if (genericTable.baseLocation() != null && !genericTable.baseLocation().isEmpty()) {
+ tableProperties.put(TABLE_PATH_KEY, genericTable.baseLocation());
+ } else if (hasLocationClause) {
+ tableProperties.put(TABLE_PATH_KEY, properties.get(TableCatalog.PROP_LOCATION));
+ }
+ }
+ return tableProperties;
+ }
+
+ /**
+ * Load spark table using DataSourceV2.
+ *
+ * @return V2Table if DataSourceV2 is available for the table format. For delta table, it returns
+ * DeltaTableV2.
+ */
+ public static Table loadV2SparkTable(GenericTable genericTable) {
+ SparkSession sparkSession = SparkSession.active();
+ TableProvider provider =
+ DataSource.lookupDataSourceV2(genericTable.format(), sparkSession.sessionState().conf())
+ .get();
+ Map<String, String> tableProperties = normalizeTablePropertiesForLoadSparkTable(genericTable);
+ return DataSourceV2Utils.getTableFromProvider(
+ provider, new CaseInsensitiveStringMap(tableProperties), scala.Option.empty());
+ }
+
+ /**
+ * Return a Spark V1Table for formats that do not use DataSourceV2. Currently, this is being used
+ * for Hudi tables
+ */
+ public static Table loadV1SparkTable(
+ GenericTable genericTable, Identifier identifier, String catalogName) {
+ Map<String, String> tableProperties = normalizeTablePropertiesForLoadSparkTable(genericTable);
+
+ // Need full identifier in order to construct CatalogTable
+ String namespacePath = String.join(".", identifier.namespace());
+ TableIdentifier tableIdentifier =
+ new TableIdentifier(
+ identifier.name(), Option.apply(namespacePath), Option.apply(catalogName));
+
+ Builder<Tuple2<String, String>, scala.collection.immutable.Map<String, String>> mb =
+ Map$.MODULE$.newBuilder();
+ tableProperties.forEach((k, v) -> mb.$plus$eq(Tuple2.apply(k, v)));
+ scala.collection.immutable.Map<String, String> scalaOptions = mb.result();
+
+ org.apache.spark.sql.catalyst.catalog.CatalogStorageFormat storage =
+ DataSource.buildStorageFormatFromOptions(scalaOptions);
+
+ // Currently Polaris generic table does not contain any schema information, partition columns,
+ // stats, etc
+ // for now we will fill the parameters we have from polaris catalog, and let underlying client
+ // resolve the rest within its catalog implementation
+ org.apache.spark.sql.types.StructType emptySchema = new org.apache.spark.sql.types.StructType();
+ @SuppressWarnings("deprecation")
+ scala.collection.immutable.Seq<String> emptyStringSeq =
+ scala.collection.JavaConverters.asScalaBuffer(new java.util.ArrayList<String>()).toList();
+ CatalogTable catalogTable =
+ new CatalogTable(
+ tableIdentifier,
+ CatalogTableType.EXTERNAL(),
+ storage,
+ emptySchema,
+ Option.apply(genericTable.format()),
+ emptyStringSeq,
+ scala.Option.empty(),
+ genericTable.properties().getOrDefault(TableCatalog.PROP_OWNER, ""),
+ System.currentTimeMillis(),
+ -1L,
+ "",
+ scalaOptions,
+ scala.Option.empty(),
Review Comment:
All good @dimas-b. Yes, one extra column here. Same responded above. Any classes with diff, I currently have them not in the common due to using `sourceSets`.
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3357984691
##########
plugins/spark/v4.0/spark/src/main/java/org/apache/polaris/spark/utils/PolarisCatalogUtils.java:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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.utils;
+
+import com.google.common.collect.Maps;
+import java.lang.reflect.Field;
+import java.util.Map;
+import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.rest.RESTCatalog;
+import org.apache.iceberg.rest.RESTSessionCatalog;
+import org.apache.iceberg.rest.auth.OAuth2Util;
+import org.apache.iceberg.spark.SparkCatalog;
+import org.apache.polaris.spark.rest.GenericTable;
+import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.catalyst.TableIdentifier;
+import org.apache.spark.sql.catalyst.catalog.CatalogTable;
+import org.apache.spark.sql.catalyst.catalog.CatalogTableType;
+import org.apache.spark.sql.connector.catalog.Identifier;
+import org.apache.spark.sql.connector.catalog.Table;
+import org.apache.spark.sql.connector.catalog.TableCatalog;
+import org.apache.spark.sql.connector.catalog.TableProvider;
+import org.apache.spark.sql.execution.datasources.DataSource;
+import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Utils;
+import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import scala.Option;
+import scala.Tuple2;
+import scala.collection.immutable.Map$;
+import scala.collection.mutable.Builder;
+
+public class PolarisCatalogUtils {
+
+ public static final String TABLE_PROVIDER_KEY = "provider";
+ public static final String TABLE_PATH_KEY = "path";
+
+ /** Check whether the table provider is iceberg. */
+ public static boolean useIceberg(String provider) {
+ return provider == null || "iceberg".equalsIgnoreCase(provider);
Review Comment:
So the files that are not in the common all have couple spots diff due to changes either in spark or delta or iceberg. As I am using `sourceSets`, we won't not be able to have two class with same names. We could do the adapters pattern as what @snazy suggested via https://github.com/apache/polaris/pull/4535#issuecomment-4563754649. But debating if this is worth it or if there is a better way to do so. Thoughts?
--
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] Spark 4 client support [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #4612:
URL: https://github.com/apache/polaris/pull/4612#issuecomment-4628132436
@dimas-b mind take another look?
--
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] Spark 4 client support [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4612:
URL: https://github.com/apache/polaris/pull/4612#discussion_r3363579578
##########
plugins/spark/v3.5/spark/BUNDLE-LICENSE:
##########
@@ -204,7 +204,8 @@
This product bundles and includes code from Apache Iceberg.
-* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/PolarisRESTCatalog.java
+* plugins/spark/common/src/main/java/org/apache/polaris/spark/BasePolarisSparkCatalog.java
* plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/SparkCatalog.java
Review Comment:
Are these classes really "copied from Iceberg" anymore? My reading is that they are very different now ๐ค
@jbonofre @snazy : 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