Core: Fix API breakage introduced by #13191

[nastra]

#13191 added an API breakage by introducing an abstract method to BaseHTTPClient. We can actually avoid breaking the API in a similar manner as to how we did it historically in the BaseHTTPClient / RESTClient by throwing an UOE when the new parameter is actually set. Since subclasses already override/implement this new method, everything should work as expected.

[nastra]

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

[amogh-jahagirdar]

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.

[nastra]

@RussellSpitzer @singhpk234 @amogh-jahagirdar can you guys please review this one? CI should be fixed once #13385 is in

[amogh-jahagirdar]

Thanks for catching and fixing this @nastra , in hindsight I agree there's really no reason that we need to break the execute API and we have a simple way to avoid it in this case.

[amogh-jahagirdar]

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.

[amogh-jahagirdar]

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

[singhpk234]

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);

[singhpk234]

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

[amogh-jahagirdar]

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.

[singhpk234]

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

[RussellSpitzer]

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?

[singhpk234]

wouldn't we doing a put on a final object then ? as part of add method, i intentionally removed the final here

[nastra]

the final applies to the actual map instance and doesn't imply that the map is immutable. Looking at the code again I don't think it actually works currently when the builder's add() method is called because data is an empty immutable map. @singhpk234 shouldn't the data map rather be just an empty mutable map?

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

[amogh-jahagirdar]

Yeah good eye, I think we would've caught this when implementing the actual client side changes for scan planning w tests, we wouldn't have any of the expected context passed through but it's good to catch this now.

[singhpk234]

LGTM as well, thanks for catching it and fix @nastra !
just have one minor comment on making the map final

[singhpk234]

Thanks @nastra really appreciate it !

[RussellSpitzer]

I approve of this, I just was a little curious about the change.

[amogh-jahagirdar]

Thanks @nastra , thanks @singhpk234 @RussellSpitzer @stevenzwu for reviewing. I'll go ahead and merge