Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
MonkeyCanCode commented on PR #442: URL: https://github.com/apache/polaris/pull/442#issuecomment-2524085956 > @singhpk234 1.7.1 is out now so we should be able to get this in Will update my PR for the same. -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
RussellSpitzer commented on PR #442: URL: https://github.com/apache/polaris/pull/442#issuecomment-2524077379 @singhpk234 1.7.1 is out now so we should be able to get this in -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
RussellSpitzer commented on PR #442: URL: https://github.com/apache/polaris/pull/442#issuecomment-2494789801 Took this branch and ran with 1.7.1-RC1 of Iceberg. All tests passed -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1851533111
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
can confirm its passing now, I have pointed to 1.8.0 artifact (can't find
1.7.1 in nightly) from nigthly, will change this 1.7.1 when it get released
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1851536407
##
settings.gradle.kts:
##
@@ -69,6 +69,7 @@ dependencyResolutionManagement {
repositories {
mavenCentral()
gradlePluginPortal()
+maven(url = "https://repository.apache.org/content/repositories/snapshots";)
Review Comment:
remove this when 1.7.1 is release please ref this thread :
https://github.com/apache/polaris/pull/442#discussion_r1851533111
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442: URL: https://github.com/apache/polaris/pull/442#discussion_r1851535590 ## gradle/libs.versions.toml: ## @@ -19,7 +19,7 @@ [versions] hadoop = "3.4.0" -iceberg = "1.6.1" +iceberg = "1.8.0-SNAPSHOT" Review Comment: change to 1.7.1 when released please ref this discussion thread https://github.com/apache/polaris/pull/442#discussion_r1851533111 -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1851533111
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
can confirm its passing now, I have pointed to 1.8.0 artifact from nigthly,
will change this 1.7.1 when it get released
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1851533111
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
can confirm its passing now, I have pointed to 1.8.0 artifact from nigthly
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1848789796
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
+1, I think it's fair to wait as we not are chasing a release time in
Polaris and 1.7.1 should be fast, I saw an RC is cut already
https://github.com/apache/iceberg/pull/11593
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
flyrain commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1848780835
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
It seems the revert PR (https://github.com/apache/iceberg/pull/11574) is
targeting Iceberg 1.7.1. Should we wait for the 1.7.1 release to avoid
unnecessary back-and-forth?
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1845271486
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
oops looks like it was a style miss :
https://github.com/apache/polaris/actions/runs/11859822454/job/33055322598
Here are the local results :
https://github.com/user-attachments/assets/9c1451a6-e0e2-48a2-b257-308a85c5c101";>
error : `org.apache.iceberg.exceptions.NoSuchNamespaceException: Namespace
does not exist: ns1?ns1a`
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1845271486
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
oops looks like it was a style miss :
https://github.com/apache/polaris/actions/runs/11859822454/job/33055322598
let me rerun style and test locally first
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1844741928
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
didn't work that great
https://github.com/apache/polaris/pull/442/commits/7bfa42d1b3ea8e0d06e536a9369ad8d736e7739a
Tracking ticket in iceberg :
https://github.com/apache/iceberg/issues/11539
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1844741928
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
didn;t work that great
https://github.com/apache/polaris/pull/442/commits/7bfa42d1b3ea8e0d06e536a9369ad8d736e7739a
Tracking ticket :
https://github.com/apache/iceberg/issues/11539
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
flyrain commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1844746714
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
Hi @singhpk234 , What's the error in
https://github.com/apache/polaris/commit/7bfa42d1b3ea8e0d06e536a9369ad8d736e7739a?
Couldn't get it once the new pipeline started.
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
nastra commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1843659550
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
changing
https://github.com/apache/polaris/blob/ce3634c80f5b97daa8410be49c1fd4d2ff363e74/polaris-service/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java#L183
to `RESTUtil.decodeNamespace(namespace)` should fix 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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
nastra commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1843603794
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
I believe the server is missing a call to `RESTUtil.decodeNamespace(..)`,
hence you're getting the encoded namespace here
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
flyrain commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1842756769
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
This is interesting, as Iceberg still uses dot to join different level of
namespaces,
https://github.com/apache/iceberg/blob/09634857e4a1333f5dc742d1dca3921e9a9f62dd/api/src/main/java/org/apache/iceberg/catalog/Namespace.java#L97-L97.
In this case, `Namespace.of("top_level", "whoops")` should be referred to
`top_level.whoops`, unless the rest util somehow squash two levels together
with the "%1F".
@singhpk234 , could you take a 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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
MonkeyCanCode commented on PR #442: URL: https://github.com/apache/polaris/pull/442#issuecomment-2475397528 Here is a PR for the 2 missing changes: https://github.com/apache/polaris/pull/447 -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
collado-mike commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1841397103
##
polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##
@@ -2055,8 +2054,6 @@ private List
listTableLike(PolarisEntitySubType subType, Namesp
*/
private FileIO loadFileIO(String ioImpl, Map properties) {
Map propertiesWithS3CustomizedClientFactory = new
HashMap<>(properties);
-propertiesWithS3CustomizedClientFactory.put(
-S3FileIOProperties.CLIENT_FACTORY,
PolarisS3FileIOClientFactory.class.getName());
Review Comment:
I think it was only ever used for this purpose, so we're probably fine
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
Is this the response in the error message now? I think the dot in the error
is much more user friendly
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
flyrain commented on PR #442: URL: https://github.com/apache/polaris/pull/442#issuecomment-2474241277 I will merge this by EOD if there is no objection. -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
snazy commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1839781418
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
There's no more intent to make the separator configurable. At least, that's
the latest I know. There was a big discussion around this topic in August this
year.
IMHO, making the separator configurable makes things overly complicated.
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1837388297
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
found this change :
https://github.com/apache/iceberg/commit/5fc1413a5efc4419ccc081f3031325f107ccddab
looks like we changes the seperator,
pending to make it configurable https://github.com/apache/iceberg/pull/10877
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
MonkeyCanCode commented on PR #442: URL: https://github.com/apache/polaris/pull/442#issuecomment-2468994729 > > @flyrain @singhpk234 should we update iceberg 1.7.0 dependencies for "getting start" examples as well? Most of them are using 1.5.2 iceberg runtime. I can take this if not already done. > > +1, @MonkeyCanCode! Thanks for picking it up as a follow-up. > Anytime. Will PR later tonight then tag the PR here. -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
flyrain commented on PR #442: URL: https://github.com/apache/polaris/pull/442#issuecomment-2468956918 > @flyrain @singhpk234 should we update iceberg 1.7.0 dependencies for "getting start" examples as well? Most of them are using 1.5.2 iceberg runtime. I can take this if not already done. +1, @MonkeyCanCode! Thanks for picking it up as a follow-up. -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
snazy commented on PR #442: URL: https://github.com/apache/polaris/pull/442#issuecomment-2468219526 Note: Today I realized that there are two "special" changes in Iceberg/Java 1.7.0: 1. Object-storage layout - the generated paths have changed. This is relevant when IAM policies for S3 + GCS are generated. IIRC, IAM policies generated by Polaris are not yet adopted for object-storage layout (`write.object-storage.enabled=true` + data path). Before 1.7.0, the paths were generated like `s3://bucket//...`, since 1.7.0 it's `s3://bucket/...`) 2. Iceberg/Java 1.7.0 refuses to submit some requests like the multi-table-commit, if the service does not return `ConfigResponse.endpoints()` or that list is empty. -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
MonkeyCanCode commented on PR #442: URL: https://github.com/apache/polaris/pull/442#issuecomment-2467039239 @flyrain @singhpk234 should we update iceberg 1.7.0 dependencies for "getting start" examples as well? Most of them are using 1.5.2 iceberg runtime. I can take this if not already done. -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1835551655
##
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##
@@ -341,7 +341,7 @@ public void testIcebergListNamespacesNestedNotFound()
throws IOException {
sessionCatalog.listNamespaces(
sessionContext, Namespace.of("top_level", "whoops")))
.isInstanceOf(NoSuchNamespaceException.class)
- .hasMessage("Namespace does not exist: top_level.whoops");
+ .hasMessage("Namespace does not exist: top_level%1Fwhoops");
Review Comment:
Checking 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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1835546848
##
polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##
@@ -2055,8 +2054,6 @@ private List
listTableLike(PolarisEntitySubType subType, Namesp
*/
private FileIO loadFileIO(String ioImpl, Map properties) {
Map propertiesWithS3CustomizedClientFactory = new
HashMap<>(properties);
-propertiesWithS3CustomizedClientFactory.put(
-S3FileIOProperties.CLIENT_FACTORY,
PolarisS3FileIOClientFactory.class.getName());
Review Comment:
Might have to mark this deprecated first since it's a public class
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on PR #442: URL: https://github.com/apache/polaris/pull/442#issuecomment-2466353377 on debugging it, looks like we need to handle this change cleanly [1], since polaris supports more endpoint than default impl, let me fix this ! [1] https://github.com/apache/iceberg/commit/a2b8008da7bc26e03248a3560d1cc7e8499d#diff-86450612dbe323d6d06cbc3846aa1913f042eaedadc0ca027c36bfbe08d3a46c -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on PR #442: URL: https://github.com/apache/polaris/pull/442#issuecomment-2466088848 checking test failure -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442: URL: https://github.com/apache/polaris/pull/442#discussion_r1835301689 ## LICENSE: ## @@ -277,6 +277,7 @@ commons-collections:commons-collections commons-io:commons-io commons-logging:commons-logging commons-net:commons-net +dev.failsafe:failsafe Review Comment: yup, it's added as part of AWS S3InputStream retry handling via : https://github.com/apache/iceberg/pull/10433/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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
flyrain commented on code in PR #442: URL: https://github.com/apache/polaris/pull/442#discussion_r1835285690 ## LICENSE: ## @@ -277,6 +277,7 @@ commons-collections:commons-collections commons-io:commons-io commons-logging:commons-logging commons-net:commons-net +dev.failsafe:failsafe Review Comment: Question: Is it a new transitive dependency brought by iceberg 1.7? -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
flyrain commented on PR #442: URL: https://github.com/apache/polaris/pull/442#issuecomment-2466060632 LGTM with a minor style issue: ``` > The following files had format violations: src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -47,7 +47,6 @@ import·org.apache.iceberg.TableMetadata; import·org.apache.iceberg.TableMetadataParser; import·org.apache.iceberg.TableOperations; -import·org.apache.iceberg.aws.s3.S3FileIOProperties; import·org.apache.iceberg.catalog.Namespace; import·org.apache.iceberg.catalog.SupportsNamespaces; import·org.apache.iceberg.catalog.TableIdentifier; Run './gradlew :polaris-service:spotlessApply' to fix these violations. ``` -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Upgrade Iceberg 1.7.0 [polaris]
singhpk234 commented on code in PR #442:
URL: https://github.com/apache/polaris/pull/442#discussion_r1835279240
##
polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##
@@ -2055,8 +2054,6 @@ private List
listTableLike(PolarisEntitySubType subType, Namesp
*/
private FileIO loadFileIO(String ioImpl, Map properties) {
Map propertiesWithS3CustomizedClientFactory = new
HashMap<>(properties);
-propertiesWithS3CustomizedClientFactory.put(
-S3FileIOProperties.CLIENT_FACTORY,
PolarisS3FileIOClientFactory.class.getName());
Review Comment:
No longer required because of this :
https://github.com/apache/iceberg/pull/11259/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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
