Add a hashcode implementation to SchemaField #3601

azymnis · 2017-07-12T10:03:04Z

Problem: I need to compare a new schema with an existing table schema. Easiest way would be to use set comparison operators. However this is not possible with SchemaField because there is no implementation of __hash__.

Solution: Add a simple implementation of __hash__ using tuple keys. This is not the most efficient implementation of a hashcode but should probably be ok for most purposes.

googlebot · 2017-07-12T10:03:06Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

azymnis · 2017-07-12T10:04:20Z

I signed!

googlebot · 2017-07-12T10:04:21Z

CLAs look good, thanks!

dhermes · 2017-07-12T15:12:28Z

I was a bit confused by what is needed, but this example clarifies for me:

>>> class A(object):
...     def __init__(self, val):
...         self.val = val
...     def __repr__(self):
...         return 'A({!r})'.format(self.val)
...     def __eq__(self, other):
...         if not isinstance(other, A):
...             return NotImplemented
...         return other.val == self.val
...     def __ne__(self, other):
...         if not isinstance(other, A):
...             return NotImplemented
...         return other.val != self.val
...
>>> a1 = A(10)
>>> a2 = A(11)
>>> a3 = A(11)
>>> a1 == a2
False
>>> a1 == a3
False
>>> a2 == a3
True
>>> set([a1, a2, a3])
set([A(10), A(11), A(11)])

bigquery/google/cloud/bigquery/schema.py

        self.fields = fields

-    def __eq__(self, other):
+    def __key(self):


bigquery/google/cloud/bigquery/schema.py

+            self.field_type.lower(),
+            self.mode,
+            self.description,
+            self.fields)


azymnis · 2017-07-13T11:00:45Z

@dhermes @tseaver what do you think of this?

bigquery/google/cloud/bigquery/schema.py

        self.mode = mode
        self.description = description
-        self.fields = fields
+        self.fields = None if(fields is None) else tuple(fields)


azymnis · 2017-07-17T13:41:03Z

@dhermes @tseaver i modified the default arg for fields to be (). The question is what to do with how this resource is represented. Should I use () there too? This is what I did, so if you would rather have me use [] I can change that.

FYI: this broke a few tests and I'm wondering if it will break any of the actual api calls.

bigquery/google/cloud/bigquery/table.py

            info['description'] = field.description
        if field.fields is not None:
-            info['fields'] = _build_schema_resource(field.fields)
+            info['fields'] = tuple(_build_schema_resource(field.fields))


bigquery/google/cloud/bigquery/table.py

    """
    if 'fields' not in info:
-        return None
+        return ()


bigquery/google/cloud/bigquery/schema.py

+    def __hash__(self):
+        return hash(self._key())
+
+    def __repr__(self):


azymnis · 2017-07-17T16:28:49Z

Hmm looks like I broke some tests in circleci, let me go fix that

googlebot · 2017-07-17T17:11:28Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

dhermes · 2017-07-17T17:12:17Z

@azymnis We (@jonparrott, @lukesneeringer and I) just had a discussion about this and determined SchemaField should be immutable in order to be hashable.

Rather than put the burden on you, I have sent a commit into this PR to make the class immutable.

lukesneeringer

One style nitpick.

bigquery/tests/unit/test_job.py

            'schema': {'fields': [
-                {'name': 'full_name', 'type': 'STRING', 'mode': 'REQUIRED'},
-                {'name': 'age', 'type': 'INTEGER', 'mode': 'REQUIRED'},
+                {'name':


bigquery/google/cloud/bigquery/schema.py

+            self._fields,
+        )
+
+    def __eq__(self, other):


azymnis · 2017-07-17T17:45:06Z

@dhermes makes sense, thanks for the update

azymnis · 2017-07-17T18:54:44Z

Thank you all!

* Add a __hash__ implementation to SchemaField * Modify default list of subfields to be the empty tuple * Making SchemaField immutable. * Adding SchemaField.__ne__.

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Jul 12, 2017

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jul 12, 2017

dhermes reviewed Jul 12, 2017

View reviewed changes

bigquery/google/cloud/bigquery/schema.py Outdated

self.fields = fields

def __eq__(self, other):

def __key(self):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

tseaver reviewed Jul 12, 2017

View reviewed changes

dhermes added the api: bigquery Issues related to the BigQuery API. label Jul 12, 2017

dhermes reviewed Jul 13, 2017

View reviewed changes

bigquery/google/cloud/bigquery/schema.py Outdated

self.mode = mode

self.description = description

self.fields = fields

self.fields = None if(fields is None) else tuple(fields)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

azymnis commented Jul 17, 2017

View reviewed changes

bigquery/google/cloud/bigquery/table.py Outdated

info['description'] = field.description

if field.fields is not None:

info['fields'] = _build_schema_resource(field.fields)

info['fields'] = tuple(_build_schema_resource(field.fields))

This comment was marked as spam.

Sign in to view

azymnis commented Jul 17, 2017

View reviewed changes

bigquery/google/cloud/bigquery/table.py

"""

if 'fields' not in info:

return None

return ()

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

azymnis commented Jul 17, 2017

View reviewed changes

bigquery/google/cloud/bigquery/schema.py

def __hash__(self):

return hash(self._key())

def __repr__(self):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

lukesneeringer approved these changes Jul 17, 2017

View reviewed changes

dhermes force-pushed the azymnis/schema_hashcode branch from 5dc1f46 to 66a3d66 Compare July 17, 2017 17:11

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jul 17, 2017

lukesneeringer suggested changes Jul 17, 2017

View reviewed changes

Argyris Zymnis and others added 6 commits July 17, 2017 10:33

Add a hashcode implementation to SchemaField

7c015fc

Rename key method, add docstring, store fields as tuples

3b5c7ba

Modify default list of subfields to be the empty tuple

1974553

Fix linting and remove unused if statement

b650702

Fix a typo

5a6a4a0

Making SchemaField immutable.

2f728fa

Reducing the number of "secondary" changes caused by SchemaField change.

8882df3

dhermes force-pushed the azymnis/schema_hashcode branch from 66a3d66 to 8882df3 Compare July 17, 2017 17:33

dhermes added 2 commits July 17, 2017 10:34

Getting rid of spurious empty line in SchemaField.mode docstring.

3a0ad42

Adding SchemaField.__ne__.

c4f4234

theacodes approved these changes Jul 17, 2017

View reviewed changes

dhermes merged commit 59fd1e4 into googleapis:master Jul 17, 2017

azymnis deleted the azymnis/schema_hashcode branch July 17, 2017 18:53

Add a hashcode implementation to SchemaField #3601

Add a hashcode implementation to SchemaField #3601

Uh oh!

Conversation

azymnis commented Jul 12, 2017

Uh oh!

googlebot commented Jul 12, 2017

Uh oh!

azymnis commented Jul 12, 2017

Uh oh!

googlebot commented Jul 12, 2017

Uh oh!

dhermes commented Jul 12, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

azymnis commented Jul 13, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

azymnis commented Jul 17, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

azymnis commented Jul 17, 2017

Uh oh!

googlebot commented Jul 17, 2017

Uh oh!

dhermes commented Jul 17, 2017

Uh oh!

lukesneeringer left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

azymnis commented Jul 17, 2017

Uh oh!

azymnis commented Jul 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants