HackPoint commented on PR #44783: URL: https://github.com/apache/arrow/pull/44783#issuecomment-3070370780
> @HackPoint, I do think it's important to add cancellation tokens to methods which themselves call other methods that already take a cancellation token -- particularly when the method in question is a public API. Even if no one ever ends up using it, the cost is quite small. By contrast, if one needs to be added later then we end up needing to override the method in order to preserve backwards binary compatibility. > > I've gone through and figured out which methods fall into this category and published a version of this PR that includes those changes to https://github.com/CurtHagenlocher/arrow/tree/flight-sql-client-changes. I can also submit a PR from that branch to your branch if that helps. In addition to the cancellation token changes (which are the largest part of the delta), I've also implemented `IEquatable<>` on `Transaction` and `IAsyncDisposable` on `PreparedStatement`. > > One thing I didn't do was to resolve the question of the unused `_dataSetSchema` and `_parameterSchema` fields on `PreparedStatement`. Work is being done to populate these, but they are never used. I started removing them but then realized that I don't really understand the difference between these and the `GetSchemaAsync` call on the same class. Can you take another look and decide what you think should be done about this? Hi Curt, Thanks a lot for pushing those updates—adding CancellationToken parameters and the extra interfaces makes perfect sense. Could you please go ahead and open a PR from flight-sql-client-changes to my branch? That will let me review and integrate your changes right away. I’ll take another pass over _dataSetSchema and _parameterSchema while I’m at it. My initial hunch is that _dataSetSchema may be redundant, but _parameterSchema is probably still necessary for optional parameter binding, so I’ll verify that and adjust accordingly. Once I’ve incorporated any tweaks you need, I’d appreciate it if you could merge my original PR as soon as everything looks good on your end. Thanks again for the thorough improvements! -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
