liurenjie1024 commented on PR #645: URL: https://github.com/apache/iceberg-rust/pull/645#issuecomment-2453287068
> @Xuanwo, @liurenjie1024 ready for another round. Ditched both the trait and the enum. Bare with me: > > As already mentioned earlier, the enum or the trait were just a vehicle to reduce duplication of logic and to clarify that there are three variants of partition specs with different nuances. However the enum should rarely be used directly, as individual variants are much more powerful with a clear hierarchy: Bound > Schemaless > Unbound. Thus, it would be bad to ship the enum around to functions that require the Bound or Schemaless variants (i.e. need a field_id). It would always require matching and error handling. Thus, we use the specific variants pretty much everywhere anyway. > > If we agree that we shouldn't use the enum to pass PartitionSpecs, then having an enum with owned fields is bad, because it requires a lot of `clone`. This is why I oped for `enum<'a> PartitionSpec<'a>` in [9562d22](https://github.com/apache/iceberg-rust/commit/9562d22a2cc550ecaf04cb857b0ad15e67c94603). > > I didn't like the enum though as I didn't want to use it anyway. So in my last commit I got rid of that as well. We gain a lot less code and API surface. We loose quite a bit of interoperability between different schema variants. What especially hurts is the `is_compatible_with` method, which used to be completely cross-functional for all variants. I implement it now only for the single combination needed by the `TableMetadataBuilder` now (Schemaless compatible with Unbound) > > Let me know what you think! Sorry I missed this comment. I agree that trait/enum just for reducing duplication of logic is somehow to antipattern, and sometimes a little duplication is acceptable. -- 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
