Re: [PR] GH-37891: [C++][Parquet] Refine several classes in Parquet encryption [arrow]
conbench-apache-arrow[bot] commented on PR #46202: URL: https://github.com/apache/arrow/pull/46202#issuecomment-2989200821 After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 0bd4e73cdbb31e48cc5a678062d82bd3957308bc. There were 118 benchmark results with an error: - Commit Run on `arm64-t4g-2xlarge-linux` at [2025-06-19 13:57:26Z](https://conbench.ursa.dev/compare/runs/23d67e28ec2e4cc0911d773b94f6d40c...d2b1063e83c847a3b62dfe90c43fc3d3/) - [`tpch` (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-07, scale_factor=1](https://conbench.ursa.dev/benchmark-results/06854291e5e47f4f8000252445b37e66) - [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-09, scale_factor=1](https://conbench.ursa.dev/benchmark-results/0685429d63fa76af8000b76b8a2edc98) - and 116 more (see the report linked below) There were no benchmark performance regressions. 🎉 The [full Conbench report](https://github.com/apache/arrow/runs/4778990) has more details. -- 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]
Re: [PR] GH-37891: [C++][Parquet] Refine several classes in Parquet encryption [arrow]
pitrou merged PR #46202: URL: https://github.com/apache/arrow/pull/46202 -- 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]
Re: [PR] GH-37891: [C++][Parquet] Refine several classes in Parquet encryption [arrow]
pitrou commented on PR #46202: URL: https://github.com/apache/arrow/pull/46202#issuecomment-2987082720 I've rebased on git main, will merge if CI passes -- 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]
Re: [PR] GH-37891: [C++][Parquet] Refine several classes in Parquet encryption [arrow]
adamreeve commented on PR #46202: URL: https://github.com/apache/arrow/pull/46202#issuecomment-2974703644 > @adamreeve any thoughts on this? This sounds good to me, but given this is technically a public class, maybe we should first mark the constructors that accept an owned pointer as deprecated and then remove them in a later release? Although I doubt this change would affect many, if any, people so I wouldn't be against just removing them. -- 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]
Re: [PR] GH-37891: [C++][Parquet] Refine several classes in Parquet encryption [arrow]
kapoisu commented on PR #46202: URL: https://github.com/apache/arrow/pull/46202#issuecomment-2969124537 I noticed that the FileKeyUnwrapper class has public constructors that accept either owned or non-owned pointer to KeyToolkit, originating from [PR #40329](https://github.com/apache/arrow/pull/40329). Internally, the implementation only uses the non-owned pointer, but it keeps a copy of shared_ptr when the owned version is used, to avoid dropping the reference count to 0. If this class is intended for internal use only, there doesn’t seem to be a strong reason to keep the owned versions. Removing them would make the semantics clearer, as users would then understand they are responsible for managing the lifetime of the object. It would also simplify future additions by avoiding the need to introduce two constructors for every new parameter combination. @adamreeve any thoughts on this? -- 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]
Re: [PR] GH-37891: [C++][Parquet] Refine several classes in Parquet encryption [arrow]
kapoisu commented on PR #46202: URL: https://github.com/apache/arrow/pull/46202#issuecomment-2968285870 > @kapoisu Were you planning to add more changes to this PR? Otherwise I think it can be merged. I shared some additional opinions in the original issue, but haven’t received a response from the original author yet. I’d like to get some feedback from others on that first. Besides, the original issue isn’t limited to parquet namespace. It just happens that my current modifications are still within that scope. If we try to cover all the necessary changes for the entire project in this PR, it might result in at least hundreds of commits to review. -- 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]
Re: [PR] GH-37891: [C++][Parquet] Refine several classes in Parquet encryption [arrow]
pitrou commented on PR #46202: URL: https://github.com/apache/arrow/pull/46202#issuecomment-2966494364 @kapoisu Were you planning to add more changes to this PR? Otherwise I think it can be merged. -- 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]
