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

falsy value of zero #249

Merged
merged 2 commits into from
Jan 4, 2024
Merged

falsy value of zero #249

merged 2 commits into from
Jan 4, 2024

Conversation

MehulBatra
Copy link
Contributor

@MehulBatra MehulBatra commented Jan 3, 2024

Resolves: 232

Python, certain values are considered False in a boolean context. These include None, 0, empty sequences/collections (''[]{}), and others. This is often referred to as “falsy” values.

With the current implementation of ->

def current_snapshot(self) -> Optional[Snapshot]
      if snapshot_id := self.metadata.current_snapshot_id:
      

The walrus operator in the if condition will assign a value to snapshot_id, but it will marked as false in case of zero and None hence will skip snapshot_id = 0 as well, to solve this raise this PR

…de None,0, empty sequences/collections, we are explicity handling that as in our case 0 stands for true
@MehulBatra
Copy link
Contributor Author

Tested Locally all three seems to be passing


class DummyClass:
    def __init__(self, metadata):
        self.metadata = metadata

def test_snapshot_id_condition():
    import unittest

    class TestMyClass(unittest.TestCase):
        def test_snapshot_id_truthy(self):
            instance = DummyClass(metadata={'current_snapshot_id': 42})
            self.assertTrue((snapshot_id := instance.metadata.get('current_snapshot_id')) is not None or isinstance(snapshot_id, int))

        def test_snapshot_id_falsy(self):
            instance = DummyClass(metadata={'current_snapshot_id': None})
            self.assertFalse((snapshot_id := instance.metadata.get('current_snapshot_id')) is not None or isinstance(snapshot_id, int))

        def test_snapshot_id_zero(self):
            instance = DummyClass(metadata={'current_snapshot_id': 0})
            self.assertTrue((snapshot_id := instance.metadata.get('current_snapshot_id')) is not None or isinstance(snapshot_id, int))

    return unittest.TestLoader().loadTestsFromTestCase(TestMyClass)


if __name__ == '__main__':
    unittest.TextTestRunner().run(test_snapshot_id_condition())

image

Co-authored-by: Fokko Driesprong <fokko@apache.org>
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.

Thanks @MehulBatra for working on this!

@Fokko Fokko merged commit 452238e into apache:main Jan 4, 2024
6 checks passed
@MehulBatra MehulBatra changed the title Bug fix falsy value of zero falsy value of zero Jan 4, 2024
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Jan 13, 2024
* certain values are considered False in a boolean context. These include None,0, empty sequences/collections, we are explicity handling that as in our case 0 stands for true

* Update pyiceberg/table/__init__.py

Co-authored-by: Fokko Driesprong <fokko@apache.org>

---------

Co-authored-by: Fokko Driesprong <fokko@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: The current snapshot with id 0 will be skip.
2 participants