rdblue commented on pull request #1409: URL: https://github.com/apache/iceberg/pull/1409#issuecomment-685115239
I think there's a slight problem with the approach that this uses. This creates a builder that interacts with the public `Catalog` API. That's great for users of the API because it is easier to create tables using the builder. But it doesn't help implementers of the API or handle the problem that we keep adding new variations of `createTable` when we add new table metadata. Since the builder ends up calling `createTable` with all of the metadata options it has accumulated, when we need to add a new one, like `distribute by` for example, we will still need to create another variant of `createTable` and update the builder to call it. So we will still accumulate more methods in the public API. Ideally, the builder would create `TableMetadata` and pass it into the implementation. The problem is that `TableMetadata` is not an API class. So if we wanted to do that, we wouldn't be able to provide the builder implementation in the API. We would have to add an interface for the builder that is implemented in some `BaseCatalog` in `iceberg-core`. That's a bit more work, but I think it would help in the long term. What do you think @aokolnychyi? ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
