Skip to content

Core: Fix API breakage introduced by #13191 #13386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
amogh-jahagirdar merged 1 commit into from
Jun 25, 2025
Merged

Core: Fix API breakage introduced by #13191 #13386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions .palantir/revapi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,12 +1285,6 @@ acceptedBreaks:
- code: "java.field.removedWithConstant"
old: "field org.apache.iceberg.TableProperties.ROW_LINEAGE"
justification: "Removing deprecations for 1.10.0"
- code: "java.method.abstractMethodAdded"
new: "method <T extends org.apache.iceberg.rest.RESTResponse> T org.apache.iceberg.rest.BaseHTTPClient::execute(org.apache.iceberg.rest.HTTPRequest,\
\ java.lang.Class<T>, java.util.function.Consumer<org.apache.iceberg.rest.responses.ErrorResponse>,\
\ java.util.function.Consumer<java.util.Map<java.lang.String, java.lang.String>>,\
\ org.apache.iceberg.rest.ParserContext)"
justification: "Add context aware parsing"
- code: "java.method.removed"
old: "method boolean org.apache.iceberg.TableMetadata::rowLineageEnabled()"
justification: "Removing deprecations for 1.10.0"
Expand Down
10 changes: 8 additions & 2 deletions core/src/main/java/org/apache/iceberg/rest/BaseHTTPClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,10 +149,16 @@ protected abstract <T extends RESTResponse> T execute(
Consumer<ErrorResponse> errorHandler,
Consumer<Map<String, String>> responseHeaders);

protected abstract <T extends RESTResponse> T execute(
protected <T extends RESTResponse> T execute(
Copy link
Member

@RussellSpitzer RussellSpitzer Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this actually stop a user from being broken on upgrade? If you extend this class and execute is called without the parserContext it still breaks but now you don't know that until runtime?

I may misunderstand the full range of possibilities here, but isn't the internal Iceberg SDK passing through the ParserContext here?

HTTPRequest request,
Class<T> responseType,
Consumer<ErrorResponse> errorHandler,
Consumer<Map<String, String>> responseHeaders,
ParserContext parserContext);
ParserContext parserContext) {
if (null != parserContext) {
throw new UnsupportedOperationException("Parser context is not supported");
}

return execute(request, responseType, errorHandler, responseHeaders);
Comment on lines +158 to +162
Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is a more elegant way to completely avoid the API breakage.

Copy link
Contributor

@singhpk234 singhpk234 Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't the execute without parserContext still abstract ? except the parser context being package protected ?

  protected abstract <T extends RESTResponse> T execute(
      HTTPRequest request,
      Class<T> responseType,
      Consumer<ErrorResponse> errorHandler,
      Consumer<Map<String, String>> responseHeaders);

Copy link
Contributor

@singhpk234 singhpk234 Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the intention to just have one abstract method for execute ? I agree with the ParserContext being made public

Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the intention to just have one abstract method for execute

@singhpk234 I think it's more in general that when there's an opportunity to avoid public classes being forced to implement something on upgrade we should try do it that way since we can establish an opinionated default implementation; it's not always possible but here I think it is.

So specifically prior to this change client implementations were already forced to implement execute without the parser context, and after #13191 they would have to implement an additional execute. Since there was an easy way to avoid that by just having a non-abstract implementation which still just fails at runtime, it seems worth it to avoid any existing client implementations from having to just override it themselves.

Copy link
Contributor

@singhpk234 singhpk234 Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That make sense ! thank you for the explanation @amogh-jahagirdar !

}
}
9 changes: 3 additions & 6 deletions core/src/main/java/org/apache/iceberg/rest/ParserContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@
import java.util.Collections;
import java.util.Map;
import org.apache.hadoop.util.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;

class ParserContext {
public class ParserContext {
Copy link
Contributor Author

@nastra nastra Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the class is being used in public API methods, therefore it should be public

Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah technically anyone can use the packaged HttpClient independently make a request on their own and pass through their own ParserContext. So from an API design perspective I agree, we should just make this public to enable people to be able to just build their own if they want. Iceberg implementation can take an internal opinionated approach for whatever client it initializes internally, but it's always available for a user to specify their own context if they so choose.


private final Map<String, Object> data;

Expand All @@ -44,11 +45,7 @@ static Builder builder() {
}

static class Builder {
private Map<String, Object> data;

private Builder() {
this.data = Collections.emptyMap();
}
private final Map<String, Object> data = Maps.newHashMap();

public Builder add(String key, Object value) {
Preconditions.checkNotNull(key, "Key cannot be null");
Expand Down