liurenjie1024 commented on PR #539: URL: https://github.com/apache/iceberg-rust/pull/539#issuecomment-2325423680
Hi, @AndreMouche Sorry for late reply. It seems that the benchmark result shows that there is no critial performance change? > Now it seems that we could use ArrowSchemaConverter in visitor pattern, could we make visit_type, visit_list as the as the function to ArrowSchemaConverter ? Sory I don't get your point. `visit_type`/`visit_list` are functions of `SchemaVisitor`, how could we make it part of `ArrowSchemaConverter`. > For example, we already know list should be a struct List, why we do not take the element_field of the list as the arguments directly? I think this is a good suggestion. However, there are several kinds of lists in arrow, for example `List`, `LargeList`, `FixedSizeList`, so I think current design is a tradeoff so that we don't need to deal with all variants. > Meanwhile, since ArrowSchemaVisitor is a general interface, could you please make the definitions of each interface function be made clearer? which will be very helpful for new newcomers like me, thanks. I totally agree that we should add some doc for new contributors. -- 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
