aokolnychyi commented on pull request #2240: URL: https://github.com/apache/iceberg/pull/2240#issuecomment-786351720
> So basically MetricsConfig.fromProperties becomes MetricsConfig.forTable so that it can be based on both the properties and the sort order for a table. The problem is that we build `MetricsConfig` in places where we don't have access to the table object. We made a recent change to make `BaseTable` serializable but we would need to change quite some places to pass it around. Also, I am not sure about a potential performance penalty. We do serialize things like spec and props so maybe it won't be a big deal to serialize the complete table object but someone has to test that. We could pass the sort order around but it anyway seemed like a major work to redo that in all places. Plus, it is easy to miss something. For example, the current change does not capture Flink but all tests are green. > The advantage of that approach is that the sort order doesn't make changes to table properties. Those are always configured by users. I think having something like `toTableProperties` on `MetricsConfig` will make it possible to migrate to column ids instead of names in the future. If we want to use column ids, then modifying table props seems inevitable to me. ---------------------------------------------------------------- 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]
