You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "electrum (via GitHub)" <gi...@apache.org> on 2023/03/12 06:59:32 UTC
[GitHub] [iceberg] electrum opened a new pull request, #7088: Core: Allow passing identity object through RESTSessionCatalog
electrum opened a new pull request, #7088:
URL: https://github.com/apache/iceberg/pull/7088
Trino needs to be able to create the `FileIO` instance with its own identity object.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #7088: Core: Allow passing identity object through RESTSessionCatalog
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #7088:
URL: https://github.com/apache/iceberg/pull/7088#issuecomment-1466586171
This looks good, except that I think we should pass `SessionContext` rather than the wrapped identity directly.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] electrum commented on pull request #7088: Core: Allow passing identity object through RESTSessionCatalog
Posted by "electrum (via GitHub)" <gi...@apache.org>.
electrum commented on PR #7088:
URL: https://github.com/apache/iceberg/pull/7088#issuecomment-1466632758
Thanks for the review. Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] electrum commented on a diff in pull request #7088: Core: Allow passing identity object through RESTSessionCatalog
Posted by "electrum (via GitHub)" <gi...@apache.org>.
electrum commented on code in PR #7088:
URL: https://github.com/apache/iceberg/pull/7088#discussion_r1134412127
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -765,24 +765,24 @@ private String fullTableName(TableIdentifier ident) {
return String.format("%s.%s", name(), ident);
}
- private FileIO newFileIO(Map<String, String> properties) {
+ private FileIO newFileIO(SessionContext context, Map<String, String> properties) {
if (null != ioBuilder) {
- return ioBuilder.apply(properties);
+ return ioBuilder.apply(context, properties);
} else {
String ioImpl =
properties.getOrDefault(CatalogProperties.FILE_IO_IMPL, ResolvingFileIO.class.getName());
return CatalogUtil.loadFileIO(ioImpl, properties, conf);
}
}
- private FileIO tableFileIO(Map<String, String> config) {
- if (config.isEmpty()) {
+ private FileIO tableFileIO(SessionContext context, Map<String, String> config) {
+ if (config.isEmpty() && context.wrappedIdentity() == null) {
Review Comment:
With the change to pass the full `SessionContext` to the builder, does this check for wrapped identity still make sense? Maybe this should be
```java
if (config.isEmpty() || ioBuilder != null)
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] electrum commented on a diff in pull request #7088: Core: Allow passing identity object through RESTSessionCatalog
Posted by "electrum (via GitHub)" <gi...@apache.org>.
electrum commented on code in PR #7088:
URL: https://github.com/apache/iceberg/pull/7088#discussion_r1134412127
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -765,24 +765,24 @@ private String fullTableName(TableIdentifier ident) {
return String.format("%s.%s", name(), ident);
}
- private FileIO newFileIO(Map<String, String> properties) {
+ private FileIO newFileIO(SessionContext context, Map<String, String> properties) {
if (null != ioBuilder) {
- return ioBuilder.apply(properties);
+ return ioBuilder.apply(context, properties);
} else {
String ioImpl =
properties.getOrDefault(CatalogProperties.FILE_IO_IMPL, ResolvingFileIO.class.getName());
return CatalogUtil.loadFileIO(ioImpl, properties, conf);
}
}
- private FileIO tableFileIO(Map<String, String> config) {
- if (config.isEmpty()) {
+ private FileIO tableFileIO(SessionContext context, Map<String, String> config) {
+ if (config.isEmpty() && context.wrappedIdentity() == null) {
Review Comment:
With the change to pass the full `SessionContext` to the builder, does this check for wrapped identity still make sense? Maybe this should be
```java
if (config.isEmpty() && ioBuilder != null)
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7088: Core: Allow passing identity object through RESTSessionCatalog
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7088:
URL: https://github.com/apache/iceberg/pull/7088#discussion_r1134364649
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -188,7 +193,7 @@ public void initialize(String name, Map<String, String> unresolved) {
client, tokenRefreshExecutor(), token, expiresAtMillis(mergedProps), catalogAuth);
}
- this.io = newFileIO(mergedProps);
+ this.io = newFileIO(mergedProps, null);
Review Comment:
I'd probably make this an empty session context.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7088: Core: Allow passing identity object through RESTSessionCatalog
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7088:
URL: https://github.com/apache/iceberg/pull/7088#discussion_r1134725284
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -765,24 +765,24 @@ private String fullTableName(TableIdentifier ident) {
return String.format("%s.%s", name(), ident);
}
- private FileIO newFileIO(Map<String, String> properties) {
+ private FileIO newFileIO(SessionContext context, Map<String, String> properties) {
if (null != ioBuilder) {
- return ioBuilder.apply(properties);
+ return ioBuilder.apply(context, properties);
} else {
String ioImpl =
properties.getOrDefault(CatalogProperties.FILE_IO_IMPL, ResolvingFileIO.class.getName());
return CatalogUtil.loadFileIO(ioImpl, properties, conf);
}
}
- private FileIO tableFileIO(Map<String, String> config) {
- if (config.isEmpty()) {
+ private FileIO tableFileIO(SessionContext context, Map<String, String> config) {
+ if (config.isEmpty() && context.wrappedIdentity() == null) {
Review Comment:
Looks correct to me.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] electrum commented on a diff in pull request #7088: Core: Allow passing identity object through RESTSessionCatalog
Posted by "electrum (via GitHub)" <gi...@apache.org>.
electrum commented on code in PR #7088:
URL: https://github.com/apache/iceberg/pull/7088#discussion_r1134408639
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -123,11 +124,15 @@ Map<String, String> params() {
}
public RESTSessionCatalog() {
- this(config -> HTTPClient.builder(config).uri(config.get(CatalogProperties.URI)).build());
+ this(config -> HTTPClient.builder(config).uri(config.get(CatalogProperties.URI)).build(), null);
}
- RESTSessionCatalog(Function<Map<String, String>, RESTClient> clientBuilder) {
+ public RESTSessionCatalog(
+ Function<Map<String, String>, RESTClient> clientBuilder,
+ BiFunction<Map<String, String>, Object, FileIO> ioBuilder) {
Review Comment:
Good idea, changed
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -188,7 +193,7 @@ public void initialize(String name, Map<String, String> unresolved) {
client, tokenRefreshExecutor(), token, expiresAtMillis(mergedProps), catalogAuth);
}
- this.io = newFileIO(mergedProps);
+ this.io = newFileIO(mergedProps, null);
Review Comment:
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #7088: Core: Allow passing identity object through RESTSessionCatalog
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #7088:
URL: https://github.com/apache/iceberg/pull/7088
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7088: Core: Allow passing identity object through RESTSessionCatalog
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7088:
URL: https://github.com/apache/iceberg/pull/7088#discussion_r1134360401
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -123,11 +124,15 @@ Map<String, String> params() {
}
public RESTSessionCatalog() {
- this(config -> HTTPClient.builder(config).uri(config.get(CatalogProperties.URI)).build());
+ this(config -> HTTPClient.builder(config).uri(config.get(CatalogProperties.URI)).build(), null);
}
- RESTSessionCatalog(Function<Map<String, String>, RESTClient> clientBuilder) {
+ public RESTSessionCatalog(
+ Function<Map<String, String>, RESTClient> clientBuilder,
+ BiFunction<Map<String, String>, Object, FileIO> ioBuilder) {
Review Comment:
I think that this should pass the `SessionContext` back rather than the wrapped identity. The session context was created by the caller anyway.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] electrum commented on a diff in pull request #7088: Core: Allow passing identity object through RESTSessionCatalog
Posted by "electrum (via GitHub)" <gi...@apache.org>.
electrum commented on code in PR #7088:
URL: https://github.com/apache/iceberg/pull/7088#discussion_r1134416703
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -765,24 +765,24 @@ private String fullTableName(TableIdentifier ident) {
return String.format("%s.%s", name(), ident);
}
- private FileIO newFileIO(Map<String, String> properties) {
+ private FileIO newFileIO(SessionContext context, Map<String, String> properties) {
if (null != ioBuilder) {
- return ioBuilder.apply(properties);
+ return ioBuilder.apply(context, properties);
} else {
String ioImpl =
properties.getOrDefault(CatalogProperties.FILE_IO_IMPL, ResolvingFileIO.class.getName());
return CatalogUtil.loadFileIO(ioImpl, properties, conf);
}
}
- private FileIO tableFileIO(Map<String, String> config) {
- if (config.isEmpty()) {
+ private FileIO tableFileIO(SessionContext context, Map<String, String> config) {
+ if (config.isEmpty() && context.wrappedIdentity() == null) {
Review Comment:
I updated the check to use the above.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] electrum commented on a diff in pull request #7088: Core: Allow passing identity object through RESTSessionCatalog
Posted by "electrum (via GitHub)" <gi...@apache.org>.
electrum commented on code in PR #7088:
URL: https://github.com/apache/iceberg/pull/7088#discussion_r1134412127
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -765,24 +765,24 @@ private String fullTableName(TableIdentifier ident) {
return String.format("%s.%s", name(), ident);
}
- private FileIO newFileIO(Map<String, String> properties) {
+ private FileIO newFileIO(SessionContext context, Map<String, String> properties) {
if (null != ioBuilder) {
- return ioBuilder.apply(properties);
+ return ioBuilder.apply(context, properties);
} else {
String ioImpl =
properties.getOrDefault(CatalogProperties.FILE_IO_IMPL, ResolvingFileIO.class.getName());
return CatalogUtil.loadFileIO(ioImpl, properties, conf);
}
}
- private FileIO tableFileIO(Map<String, String> config) {
- if (config.isEmpty()) {
+ private FileIO tableFileIO(SessionContext context, Map<String, String> config) {
+ if (config.isEmpty() && context.wrappedIdentity() == null) {
Review Comment:
With the change to pass the full `SessionContext` to the builder, does this check for wrapped identity still make sense? Maybe this should be
```java
if (config.isEmpty() && ioBuilder == null)
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org