rdblue commented on pull request #1346: URL: https://github.com/apache/iceberg/pull/1346#issuecomment-698666137
I'm going ahead and merging this because I think the remaining things are minor, but we will probably want to fix them. First, breaking the tests across two classes could be cleaner. In the future, I'd recommend writing tests and not breaking across a parent/child class until you want to add the other set of tests. That way it is easier to see what is needed and will be clean. Second, I don't understand the benefit of having a separate `ScanOptions` class, besides that it can pull options out of a map. It seems to me that it would be simpler to just have the `FlinkSource.Builder` class and move all of the scan options to that class. ---------------------------------------------------------------- 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: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
