nk1506 commented on PR #9852: URL: https://github.com/apache/iceberg/pull/9852#issuecomment-2189554813
Thanks a lot @szehon-ho . > hi, @nk1506 thanks for ping, ill look at this, but just FYI, when I look a few times back this patch seems a bit far, it adds things like ViewAwareTableBuilder and TableAwareViewBuilder that dont make much sense, and it will take some time to think if there is a better way. Since other catalogs have followed the same strategy, I kept those changes out of this PR's scope. > > Also, because of the earlier review feedback from @nastra it seems we abandoned the effort to unify metadata class to re-use code from HiveTableOperations that I and @pvary initially proposed . If this is the direction we chose, its ok , but it will take personally some time for me to double check everything, as we are re-implementing all the commit code. As refactoring with common code for table and view this PR became very big. and it was being difficult to review. We definitely want to bring the common part together. I think we should do it with another patch. > > Another reviewer may also look at the patch of course, I am just sharing my experience reviewing this one. @nastra has reviewed throughly on the HiveCatalog and related parts. He proposed to get feedback on the Hive core parts from experts. -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
