Skip to content
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

Make ColumnSelector.all a property instead of a manually set attribute #296

Merged
merged 4 commits into from
May 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion merlin/dag/selector.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ def __init__(
subgroups: List["ColumnSelector"] = None,
tags: List[Union[Tags, str]] = None,
):
self._all = False
self._names = names if names is not None else []
self._tags = tags if tags is not None else []
self.subgroups = subgroups if subgroups is not None else []

self.all = isinstance(names, str) and names == "*"
if self.all:
self._names = []
self._tags = []
Expand Down Expand Up @@ -77,6 +77,11 @@ def __init__(
self._names = plain_names
self._nested_check()

@property
def all(self):
self._all = self._all or (isinstance(self._names, str) and self._names == "*")
return self._all

@property
def tags(self):
return list(dict.fromkeys(self._tags).keys())
Expand Down
6 changes: 6 additions & 0 deletions tests/unit/dag/test_column_selector.py
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,12 @@ def test_applying_inverse_selector_to_schema_selects_relevant_columns():
assert result == schema


def test_wildcard_selector_bool():
selector = ColumnSelector("*")

assert selector.all is True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this test would have passed without this change. I'm not sure if this PR has a functional change, though the property method is clearer in terms of documenting the public interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm right there with you on thinking this shouldn't be a functional change, but if you see the linked bug ticket, somehow self.all is not getting set/created on selector objects. I think this should address that issue, although I'm not entirely sure why the code wasn't working as written.



def test_wildcard_selection():
selector = ColumnSelector("*")

Expand Down