feat: make Table.join() use the same logic as Table.rename() #10753
+65
−65
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NickCrews
force-pushed
the
join-fix-lname
branch
from
3e81ab2 to
35483fb
Compare
NickCrews
force-pushed
the
join-fix-lname
branch
from
35483fb to
4e4d811
Compare
There is this code inside expr/types/join.py:
```python
def disambiguate_fields(
how,
predicates,
equalities,
left_fields,
right_fields,
left_template,
right_template,
):
"""Resolve name collisions between the left and right tables."""
collisions = set()
left_template = left_template or "{name}"
right_template = right_template or "{name}"
...
```
This makes it so that "" is treated equivalently to "{name}". So this change doesn't have any behavior change, it just makes the defaults follow a more consistent pattern. The old patterns was a remnant from the lsuffix API we used to have.
before, any falsy values were interpreted as a noop. Now this errors.
NickCrews
force-pushed
the
join-fix-lname
branch
from
4e4d811 to
433af80
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are a few different changes here, I can split them into separate PRs if you want. I think the first two refactor commits are strictly improvements, with no behavior change (I think), and feel pretty good about them. The second two are behavior changes and could use more discussion.
lnameparam of the.join()methods, from""to"{name}". This by itself doesn't actually have any behavior change, it just is a lot more symmetrical with the other argument. This default is a remnant of when we used thelsuffixandrsuffixAPI..join(). This increases the scope of .join(), so it can accept a wider range of inputs. But makes our API more consistent. I wonder if there are other places where we could take advantage of this util method?.join(..., lname=""). Before. it would act the same as passing"{name}", now it errors. I doubt this affects many users, but probably affects some. I can adjust this in several ways (In order of my preference):util.rename()so that resolving to falsy/None is an error. Users MUST return a string that they want as the result. This is the most breaking, but also I think the most sane. I already proposed this during the old .rename() refactor, but we decided against it.util.rename()so that anything falsy is interpreted as a no-op. This is similar to how currentlyNoneis interpreted. This would not be breaking at all, but would increase the scope of both .join() and .rename()