Skip to content

Conversation

@jun-he
Copy link
Collaborator

@jun-he jun-he commented May 9, 2022

To split the PR #3450, open a new PR for void transform here.

@github-actions github-actions bot added the python label May 9, 2022
@jun-he jun-he force-pushed the jun/add-void-transform branch from e91b73a to bcaa0ce Compare May 9, 2022 06:39
@jun-he jun-he requested review from rdblue and samredai May 9, 2022 06:41
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Some small comments, but looks good in general 👍🏻

def __init__(self):
super().__init__("void", "transforms.always_null()")

def apply(self, value):
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to #4728

Suggested change
def apply(self, value):
def apply(self, value: Optional[S]) -> Optional[int]:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the comment. Updated.


_instance = None

def __new__(cls):
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add a test for this:

def test_check_if_identical():
    assert transforms.always_null() is transforms.always_null()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 Updated the test.

def result_type(self, source: IcebergType) -> IcebergType:
return source

def to_human_string(self, value) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def to_human_string(self, value) -> str:
def to_human_string(self, Optional[value]) -> str:

@rdblue
Copy link
Contributor

rdblue commented May 10, 2022

@jun-he, since #4728 was merged, I think this will fail tests. Can you update please?

@jun-he jun-he force-pushed the jun/add-void-transform branch from bcaa0ce to 7e13846 Compare May 13, 2022 18:04
@jun-he
Copy link
Collaborator Author

jun-he commented May 13, 2022

@rdblue @Fokko I rebased it and updated the PR accordingly. Can you please take a look? Thanks.

@rdblue rdblue merged commit 55bc71e into apache:master May 13, 2022
@rdblue
Copy link
Contributor

rdblue commented May 13, 2022

Thanks, @jun-he!

@jun-he jun-he deleted the jun/add-void-transform branch May 15, 2022 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants