icexelloss commented on PR #34627: URL: https://github.com/apache/arrow/pull/34627#issuecomment-1489477651
@westonpace I believe the two main remaining items are: (1) https://github.com/apache/arrow/pull/34627/files#r1151191726 Your comment here about not exposing these methods in the header. I agree with this and made an attempt here: https://github.com/rtpsw/arrow/pull/3/files I am able to move those methods internal to `relational_internal.cc` but as a result I also need to move the DefaultExtensionProvider to `relation_internal.cc` because it needs to access those method. I feel this is a bit cleaner than what is currently in the PR. If you are OK with this I can update the PR with the change. If you want some other way to resolve this / or leave it as is in the PR let me know as well. (2) https://github.com/apache/arrow/pull/34627#discussion_r1151204240 This is potentially a bug / correctness issue. Given this is not introduced in this PR I am leaning towards debug this as follow up (I actually suspect this could cause some internal issues and asked @rtpsw to test but I also don't want to drag this PR for too long). If you feel otherwise let me know I can try to get to the bottom of this. In any case, I would like to move forward with this PR so please let me know your preference of the two issues above or any other issue that I missed. -- 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]
