XJDKC commented on PR #14465: URL: https://github.com/apache/iceberg/pull/14465#issuecomment-3518882921
> Hey @XJDKC, > > Just for my information, would you mind explaining a bit more about the motivation and a more concrete use-case where this is needed? Is there a particular functionality in RESTTableOperations that you miss and would be interested in using? My initial gut feeling tells me that exposing table ops and making it injectable is a bit wild. I'm wondering what others think, though. Technically, if we want this to be injected, shouldn't we expect an interface from the API module as the input param, that is in turn implemented in the core module? > > Just an additional nit is that this PR seems to add 2 different changes: A way to inject an IOBuilder through the RESTCatalog (currently only RESTSessionCatalog has this) and a way to inject a REST ops builder. Would it make sense to split these into 2 PRs and test them separately? > would you mind explaining a bit more about the motivation and a more concrete use-case where this is needed? Is there a particular functionality in RESTTableOperations that you miss and would be interested in using? There isn’t a specific functionality missing, but for some platforms (especially those not using Spark), they often have platform-specific requirements, for example, custom logic for accessing storage, adding table-level headers, logging, or auditing. The default `RESTTableOperations` isn't designed to accommodate these platform-specific behaviors, nor should it. That's why we need to provide the ability for users to extend or replace it when necessary. > Technically, if we want this to be injected, shouldn't we expect an interface from the API module as the input param, that is in turn implemented in the core module? I'm fine with either option, the key goal is to make it injectable. I simply followed the existing pattern in the codebase (e.g., the ClientBuilder). > Just an additional nit is that this PR seems to add 2 different changes: A way to inject an IOBuilder through the RESTCatalog (currently only RESTSessionCatalog has this) and a way to inject a REST ops builder. Would it make sense to split these into 2 PRs and test them separately? You’re right that this PR introduces two related changes, but both serve the same purpose - improving injectability and extensibility. I don't see strong benefits in splitting them, since the changes are closely related and covered in the test. I don't have a strong preference. If others feel strongly about separating them for clarity or testing purposes, I'm happy to split the FileIO change into a separate PR. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
