advancedxy commented on PR #956: URL: https://github.com/apache/iceberg-cpp/pull/956#issuecomment-6120653013
I did another round, and I think the metrics filter itself is in good shape. The changes to parquet_reader.cc is correct but it do add more complexity and don't work well with future page skipping support. For that, I have discussed with agent to introduce RowPositionReader in the iceberg-cpp side first, then make it to the upstream arrrow cpp's parquet reader, which we would also make patches to support page skipping. Therefore, I think we can split this PR into 3-4 smaller ones: 1. binder's fix about IsBoundVisitor, which I mentioned 2. https://github.com/apache/iceberg-cpp/pull/995 for nullability fix 3. The metrics filter in this pr 4. The integration part of all the previous prs(and probably after the introduce of RowPositionReader, which I may submit a PR later today). WDYT @YinZheng-Sun @wgtmac -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
