[GitHub] [arrow] jorisvandenbossche commented on pull request #34654: GH-34248: [Python] Expose the order_by node

["jorisvandenbossche (via GitHub)" <gi...@apache.org>]

jorisvandenbossche commented on PR #34654:
URL: https://github.com/apache/arrow/pull/34654#issuecomment-1479556570

   > I am a bit unsure about the reason for the change in `CSortKey` class though. This class was only used in `Select*`, `Rank*` and `SortOptions` where, I guess, a field reference could have only been a string column name.
   
   Yeah, this was actually changed a while ago in C++, but the cython bindings exposing it as ``CSortKey(c_string name, ..)`` still worked fine because there is an implicit conversion from string to FieldRef possible. 
   For this PR however, I needed to create a SortKey from a FieldRef (to be consistent with the rest of the node options), and so I needed to update the constructor  to  ``CSortKey(CFieldRef ..)``, which also meant updating it in the other places. 
   
   > Then, I think, this should also be added in the docstrings for other classes that use sort_keys and add this cases to the tests? (maybe not as a part of this PR)
   
   Good point. Updated the docstrings and tests


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org