-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add keep_order
argument to table.join
for original table order
#16361
Conversation
keep_order
argument to astropy.table.join
for original table orderkeep_order
argument to table.join
for original table order
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks good. My one worry is that if the _join
itself fails (say because of a metadata conflict), an input table can be left with an extra column. So, I'd suggest to wrap the lot in a try/finally
with the special column removed in the finally
part.
On the API: an option to avoid having to swap input arguments for inner
would be to allow keep_order
to be a string ('right' or 'left'). But arguably overkill.
Good catch, will do.
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two minor remarks, otherwise looks good (and a relief if it means I can ditch #15994 😅) !
astropy/table/operations.py
Outdated
@@ -426,6 +449,13 @@ def join( | |||
keys_right=keys_right, | |||
) | |||
|
|||
if sort_table is not None: | |||
# Sort the table back to the original order and remove the temporary columns. | |||
# If sort_table is not None that implies keep_order=True. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so ... why not use if keep_order:
directly ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For keep_order=True
and join_type="outer"
then sort_table
is None
and the code would fail.
@mhvk @neutrinoceros - I think I have addressed the comments and CI is passing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, all OK now!
Description
This pull request is to allow maintaining the original table order when doing a
left
,right
orinner
table join. This is done by adding a new keyword argumentkeep_order
to thejoin
function.For
outer
orcartesian
joins there is no meaningful unique ordering so thekeep_order
argument value is ignored but a warning is issued forkeep_order=True
.Fixes #11619
Close #15994