You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2025/11/06 10:08:17 UTC
[PR] Core: Add storage credentials to PlanTableScanResponse [iceberg]
nastra opened a new pull request, #14518:
URL: https://github.com/apache/iceberg/pull/14518
(no comment)
--
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
Re: [PR] Core: Add storage credentials to PlanTableScanResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra closed pull request #14518: Core: Add storage credentials to PlanTableScanResponse
URL: https://github.com/apache/iceberg/pull/14518
--
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
Re: [PR] Core: Add storage credentials to PlanTableScanResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #14518:
URL: https://github.com/apache/iceberg/pull/14518#issuecomment-3558492072
@amogh-jahagirdar @singhpk234 I've just merged the Spec changes from https://github.com/apache/iceberg/pull/14563, so this should be ready to be reviewed/merged 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@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
Re: [PR] Core: Add storage credentials to PlanTableScanResponse [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14518:
URL: https://github.com/apache/iceberg/pull/14518#discussion_r2499970270
##########
core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponse.java:
##########
@@ -101,10 +112,26 @@ public Builder withPlanId(String id) {
return this;
}
+ public Builder withCredential(Credential credential) {
+ this.credentials.add(credential);
+ return this;
+ }
+
+ public Builder withCredentials(List<Credential> credentialsToAdd) {
+ this.credentials = credentialsToAdd;
+ return this;
+ }
Review Comment:
can we just have one API like
```java
public Builder withCredentials(List<Credential> creds) {
this.credentials.addAll(creds);
return 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@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
Re: [PR] Core: Add storage credentials to PlanTableScanResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14518:
URL: https://github.com/apache/iceberg/pull/14518#issuecomment-3567411472
Thanks @nastra for the PR! Thanks @amogh-jahagirdar @singhpk234 for the review!
--
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
Re: [PR] Core: Add storage credentials to PlanTableScanResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14518:
URL: https://github.com/apache/iceberg/pull/14518#discussion_r2502836596
##########
core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponse.java:
##########
@@ -101,10 +112,26 @@ public Builder withPlanId(String id) {
return this;
}
+ public Builder withCredential(Credential credential) {
+ this.credentials.add(credential);
+ return this;
+ }
+
+ public Builder withCredentials(List<Credential> credentialsToAdd) {
+ this.credentials = credentialsToAdd;
+ return this;
+ }
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
Re: [PR] Core: Add storage credentials to PlanTableScanResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14518:
URL: https://github.com/apache/iceberg/pull/14518#discussion_r2549645354
##########
core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponseParser.java:
##########
@@ -92,13 +105,28 @@ public static PlanTableScanResponse fromJson(
List<FileScanTask> fileScanTasks =
TableScanResponseParser.parseFileScanTasks(json, deleteFiles, specsById, caseSensitive);
- return PlanTableScanResponse.builder()
- .withPlanId(planId)
- .withPlanStatus(planStatus)
- .withPlanTasks(planTasks)
- .withFileScanTasks(fileScanTasks)
- .withDeleteFiles(deleteFiles)
- .withSpecsById(specsById)
- .build();
+ PlanTableScanResponse.Builder builder =
+ PlanTableScanResponse.builder()
+ .withPlanId(planId)
+ .withPlanStatus(planStatus)
+ .withPlanTasks(planTasks)
+ .withFileScanTasks(fileScanTasks)
+ .withDeleteFiles(deleteFiles)
+ .withSpecsById(specsById);
+
+ if (json.hasNonNull(STORAGE_CREDENTIALS)) {
+ JsonNode credsNode = JsonUtil.get(STORAGE_CREDENTIALS, json);
+ Preconditions.checkArgument(
+ credsNode.isArray(), "Cannot parse credentials from non-array: %s", credsNode);
+
+ List<Credential> credentials = Lists.newArrayList();
+ for (JsonNode credential : credsNode) {
+ credentials.add(CredentialParser.fromJson(credential));
+ }
+
+ builder.withCredentials(credentials);
Review Comment:
sure, good catch. I've updated 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@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
Re: [PR] Core: Add storage credentials to PlanTableScanResponse [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao merged PR #14518:
URL: https://github.com/apache/iceberg/pull/14518
--
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
Re: [PR] Core: Add storage credentials to PlanTableScanResponse [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14518:
URL: https://github.com/apache/iceberg/pull/14518#discussion_r2549653312
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestPlanTableScanResponseParser.java:
##########
@@ -440,4 +444,231 @@ public void roundTripSerdeWithoutDeleteFiles() {
assertThat(PlanTableScanResponseParser.toJson(copyResponse)).isEqualTo(expectedJson);
}
+
+ @Test
+ public void emptyOrInvalidCredentials() {
+ assertThat(
+ PlanTableScanResponseParser.fromJson(
+ "{\"plan-status\": \"completed\",\"storage-credentials\": null}",
+ PARTITION_SPECS_BY_ID,
+ false)
+ .credentials())
+ .isEmpty();
+
+ assertThat(
+ PlanTableScanResponseParser.fromJson(
+ "{\"plan-status\": \"completed\",\"storage-credentials\": []}",
+ PARTITION_SPECS_BY_ID,
+ false)
+ .credentials())
+ .isEmpty();
+
+ assertThatThrownBy(
+ () ->
+ PlanTableScanResponseParser.fromJson(
+ "{\"plan-status\": \"completed\",\"storage-credentials\": \"invalid\"}",
+ PARTITION_SPECS_BY_ID,
+ false))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse credentials from non-array: \"invalid\"");
+ }
+
+ @Test
+ public void roundTripSerdeWithCredentials() {
+ PlanStatus planStatus = PlanStatus.fromName("completed");
+ List<Credential> credentials =
+ ImmutableList.of(
+ ImmutableCredential.builder()
+ .prefix("s3://custom-uri")
+ .config(
+ ImmutableMap.of(
+ "s3.access-key-id",
+ "keyId",
+ "s3.secret-access-key",
+ "accessKey",
+ "s3.session-token",
+ "sessionToken"))
+ .build(),
+ ImmutableCredential.builder()
+ .prefix("gs://custom-uri")
+ .config(
+ ImmutableMap.of(
+ "gcs.oauth2.token", "gcsToken1", "gcs.oauth2.token-expires-at", "1000"))
+ .build(),
Review Comment:
yes that's possible and the spec allows this explicitly and you'd end up with `ResolvingFileIO`
--
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
Re: [PR] Core: Add storage credentials to PlanTableScanResponse [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14518:
URL: https://github.com/apache/iceberg/pull/14518#discussion_r2550891650
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestPlanTableScanResponseParser.java:
##########
@@ -440,4 +444,231 @@ public void roundTripSerdeWithoutDeleteFiles() {
assertThat(PlanTableScanResponseParser.toJson(copyResponse)).isEqualTo(expectedJson);
}
+
+ @Test
+ public void emptyOrInvalidCredentials() {
+ assertThat(
+ PlanTableScanResponseParser.fromJson(
+ "{\"status\": \"completed\",\"storage-credentials\": null}",
+ PARTITION_SPECS_BY_ID,
+ false)
+ .credentials())
+ .isEmpty();
+
+ assertThat(
+ PlanTableScanResponseParser.fromJson(
+ "{\"status\": \"completed\",\"storage-credentials\": []}",
+ PARTITION_SPECS_BY_ID,
+ false)
+ .credentials())
+ .isEmpty();
+
+ assertThatThrownBy(
+ () ->
+ PlanTableScanResponseParser.fromJson(
+ "{\"status\": \"completed\",\"storage-credentials\": \"invalid\"}",
+ PARTITION_SPECS_BY_ID,
+ false))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse credentials from non-array: \"invalid\"");
+ }
+
+ @Test
+ public void roundTripSerdeWithCredentials() {
+ PlanStatus planStatus = PlanStatus.fromName("completed");
Review Comment:
Minor: PlanStatus.Completed?
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestPlanTableScanResponseParser.java:
##########
@@ -440,4 +444,231 @@ public void roundTripSerdeWithoutDeleteFiles() {
assertThat(PlanTableScanResponseParser.toJson(copyResponse)).isEqualTo(expectedJson);
}
+
+ @Test
+ public void emptyOrInvalidCredentials() {
+ assertThat(
+ PlanTableScanResponseParser.fromJson(
+ "{\"status\": \"completed\",\"storage-credentials\": null}",
+ PARTITION_SPECS_BY_ID,
+ false)
+ .credentials())
+ .isEmpty();
+
+ assertThat(
+ PlanTableScanResponseParser.fromJson(
+ "{\"status\": \"completed\",\"storage-credentials\": []}",
+ PARTITION_SPECS_BY_ID,
+ false)
+ .credentials())
+ .isEmpty();
+
+ assertThatThrownBy(
+ () ->
+ PlanTableScanResponseParser.fromJson(
+ "{\"status\": \"completed\",\"storage-credentials\": \"invalid\"}",
+ PARTITION_SPECS_BY_ID,
+ false))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse credentials from non-array: \"invalid\"");
+ }
+
+ @Test
+ public void roundTripSerdeWithCredentials() {
+ PlanStatus planStatus = PlanStatus.fromName("completed");
+ List<Credential> credentials =
+ ImmutableList.of(
+ ImmutableCredential.builder()
+ .prefix("s3://custom-uri")
+ .config(
+ ImmutableMap.of(
+ "s3.access-key-id",
+ "keyId",
+ "s3.secret-access-key",
+ "accessKey",
+ "s3.session-token",
+ "sessionToken"))
+ .build(),
+ ImmutableCredential.builder()
+ .prefix("gs://custom-uri")
+ .config(
+ ImmutableMap.of(
+ "gcs.oauth2.token", "gcsToken1", "gcs.oauth2.token-expires-at", "1000"))
+ .build(),
+ ImmutableCredential.builder()
+ .prefix("gs")
+ .config(
+ ImmutableMap.of(
+ "gcs.oauth2.token", "gcsToken2", "gcs.oauth2.token-expires-at", "2000"))
+ .build());
+
+ PlanTableScanResponse response =
+ PlanTableScanResponse.builder()
+ .withPlanStatus(planStatus)
+ .withCredentials(credentials)
+ .withSpecsById(PARTITION_SPECS_BY_ID)
+ .build();
+
+ String expectedJson =
+ "{\n"
+ + " \"status\" : \"completed\",\n"
+ + " \"storage-credentials\" : [ {\n"
+ + " \"prefix\" : \"s3://custom-uri\",\n"
+ + " \"config\" : {\n"
+ + " \"s3.access-key-id\" : \"keyId\",\n"
+ + " \"s3.secret-access-key\" : \"accessKey\",\n"
+ + " \"s3.session-token\" : \"sessionToken\"\n"
+ + " }\n"
+ + " }, {\n"
+ + " \"prefix\" : \"gs://custom-uri\",\n"
+ + " \"config\" : {\n"
+ + " \"gcs.oauth2.token\" : \"gcsToken1\",\n"
+ + " \"gcs.oauth2.token-expires-at\" : \"1000\"\n"
+ + " }\n"
+ + " }, {\n"
+ + " \"prefix\" : \"gs\",\n"
+ + " \"config\" : {\n"
+ + " \"gcs.oauth2.token\" : \"gcsToken2\",\n"
+ + " \"gcs.oauth2.token-expires-at\" : \"2000\"\n"
+ + " }\n"
+ + " } ]\n"
+ + "}";
+
+ String json = PlanTableScanResponseParser.toJson(response, true);
+ assertThat(json).isEqualTo(expectedJson);
+
+ PlanTableScanResponse fromResponse =
+ PlanTableScanResponseParser.fromJson(json, PARTITION_SPECS_BY_ID, false);
+ PlanTableScanResponse copyResponse =
+ PlanTableScanResponse.builder()
+ .withPlanStatus(fromResponse.planStatus())
+ .withPlanId(fromResponse.planId())
+ .withSpecsById(PARTITION_SPECS_BY_ID)
+ .withCredentials(credentials)
+ .build();
+
+ assertThat(PlanTableScanResponseParser.toJson(copyResponse, true)).isEqualTo(expectedJson);
+ }
+
+ @Test
+ public void roundTripSerdeWithValidStatusAndFileScanTasksAndCredentials() {
+ ResidualEvaluator residualEvaluator =
+ ResidualEvaluator.of(SPEC, Expressions.equal("id", 1), true);
+ FileScanTask fileScanTask =
+ new BaseFileScanTask(
+ FILE_A,
+ new DeleteFile[] {FILE_A_DELETES},
+ SchemaParser.toJson(SCHEMA),
+ PartitionSpecParser.toJson(SPEC),
+ residualEvaluator);
+
+ PlanStatus planStatus = PlanStatus.fromName("completed");
Review Comment:
same as above, i think we can just use the enum 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
Re: [PR] Core: Add storage credentials to PlanTableScanResponse [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14518:
URL: https://github.com/apache/iceberg/pull/14518#discussion_r2549577468
##########
core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponseParser.java:
##########
@@ -92,13 +105,28 @@ public static PlanTableScanResponse fromJson(
List<FileScanTask> fileScanTasks =
TableScanResponseParser.parseFileScanTasks(json, deleteFiles, specsById, caseSensitive);
- return PlanTableScanResponse.builder()
- .withPlanId(planId)
- .withPlanStatus(planStatus)
- .withPlanTasks(planTasks)
- .withFileScanTasks(fileScanTasks)
- .withDeleteFiles(deleteFiles)
- .withSpecsById(specsById)
- .build();
+ PlanTableScanResponse.Builder builder =
+ PlanTableScanResponse.builder()
+ .withPlanId(planId)
+ .withPlanStatus(planStatus)
+ .withPlanTasks(planTasks)
+ .withFileScanTasks(fileScanTasks)
+ .withDeleteFiles(deleteFiles)
+ .withSpecsById(specsById);
+
+ if (json.hasNonNull(STORAGE_CREDENTIALS)) {
+ JsonNode credsNode = JsonUtil.get(STORAGE_CREDENTIALS, json);
+ Preconditions.checkArgument(
+ credsNode.isArray(), "Cannot parse credentials from non-array: %s", credsNode);
+
+ List<Credential> credentials = Lists.newArrayList();
+ for (JsonNode credential : credsNode) {
+ credentials.add(CredentialParser.fromJson(credential));
+ }
+
+ builder.withCredentials(credentials);
Review Comment:
can we do
```suggestion
builder.withCredentials(LoadCredentialsResponseParser.fromJson(json).credentials())
```
Like we do :
https://github.com/apache/iceberg/blob/f09dc3c108b746852a3febda49978ae1546797e8/core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponseParser.java#L100-L102
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestPlanTableScanResponseParser.java:
##########
@@ -440,4 +444,231 @@ public void roundTripSerdeWithoutDeleteFiles() {
assertThat(PlanTableScanResponseParser.toJson(copyResponse)).isEqualTo(expectedJson);
}
+
+ @Test
+ public void emptyOrInvalidCredentials() {
+ assertThat(
+ PlanTableScanResponseParser.fromJson(
+ "{\"plan-status\": \"completed\",\"storage-credentials\": null}",
+ PARTITION_SPECS_BY_ID,
+ false)
+ .credentials())
+ .isEmpty();
+
+ assertThat(
+ PlanTableScanResponseParser.fromJson(
+ "{\"plan-status\": \"completed\",\"storage-credentials\": []}",
+ PARTITION_SPECS_BY_ID,
+ false)
+ .credentials())
+ .isEmpty();
+
+ assertThatThrownBy(
+ () ->
+ PlanTableScanResponseParser.fromJson(
+ "{\"plan-status\": \"completed\",\"storage-credentials\": \"invalid\"}",
+ PARTITION_SPECS_BY_ID,
+ false))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse credentials from non-array: \"invalid\"");
+ }
+
+ @Test
+ public void roundTripSerdeWithCredentials() {
+ PlanStatus planStatus = PlanStatus.fromName("completed");
+ List<Credential> credentials =
+ ImmutableList.of(
+ ImmutableCredential.builder()
+ .prefix("s3://custom-uri")
+ .config(
+ ImmutableMap.of(
+ "s3.access-key-id",
+ "keyId",
+ "s3.secret-access-key",
+ "accessKey",
+ "s3.session-token",
+ "sessionToken"))
+ .build(),
+ ImmutableCredential.builder()
+ .prefix("gs://custom-uri")
+ .config(
+ ImmutableMap.of(
+ "gcs.oauth2.token", "gcsToken1", "gcs.oauth2.token-expires-at", "1000"))
+ .build(),
Review Comment:
is this possible can there be location for both S3 and GCS ? do we expect Resolving FileIO in this case ?
--
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