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

Adding Bigtable RowFilter base class. #1291

Merged
merged 1 commit into from
Dec 17, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Dec 16, 2015

Also adding regex filters for a row key (bytes) and a family name (string).

These come from

https://github.com/GoogleCloudPlatform/cloud-bigtable-client/blob/6a498cd3e660c7ed18299e980c1658d67661e69b/bigtable-protos/src/main/proto/google/bigtable/v1/bigtable_data.proto#L321-L404

In addition to more classes for primitive properties, parent classes are forthcoming to handle the non-primitive cases of filter Chain, Interleave and Condition (ternary).

Also renaming some redunant unit test names in test_column_family.py and ditching use of NotImplementedError in bigtable base classes (for both GC rule and row filter).

@dhermes dhermes added the api: bigtable Issues related to the Bigtable API. label Dec 16, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 16, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Dec 16, 2015

@tseaver As with #1290 I am very open to breaking this class with 13 possible filters into 13 separate classes, but wanted to get the pattern in full view before we make a decision in #1290.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 17, 2015

@tseaver I'm going to start with just two here.

@dhermes dhermes changed the title Adding Bigtable RowFilter. Adding Bigtable RowFilter base class. Dec 17, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Dec 17, 2015

@tseaver PTAL. I opted not to have regex filters inherit from a parent, but these ended up having also identical unit tests, so maybe it would've been worth it? I figured it was unnecessary since the classes to so little. (FWIW of the 19 filters, 4 of them are regex filters)

@tseaver
Copy link
Contributor

tseaver commented Dec 17, 2015

If four of the filters take only a regex ctor argument, and differ only in terms of how they construct the protobuf, then factoring out __init__, __eq__, and __ne__ into a base class seems like a win.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 17, 2015

OK. Will re-factor. __ne__ is already in the main base class, but the other 2 are still uniform. Will ping once done.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 17, 2015

@tseaver PTAL. I added _RegexFilter as a base class.

return False
return other.regex == self.regex

def to_pb(self):

This comment was marked as spam.

This comment was marked as spam.

Also adding regex filters for a row key (bytes) and a
family name (string).

These come from

https://github.com/GoogleCloudPlatform/cloud-bigtable-client/blob/6a498cd3e660c7ed18299e980c1658d67661e69b/bigtable-protos/src/main/proto/google/bigtable/v1/bigtable_data.proto#L321-L404

In addition to more classes for primitive properties, parent classes are
forthcoming to handle the non-primitive cases of filter Chain, Interleave
and Condition (ternary).

Also renaming some redunant unit test names in test_column_family.py and
ditching use of NotImplementedError in bigtable base classes (for
both GC rule and row filter).
@dhermes
Copy link
Contributor Author

dhermes commented Dec 17, 2015

@tseaver PTAL I dropped all the virtual to_pb methods.

@tseaver
Copy link
Contributor

tseaver commented Dec 17, 2015

LGTM

dhermes added a commit that referenced this pull request Dec 17, 2015
Adding Bigtable RowFilter base class.
@dhermes dhermes merged commit 30bec2d into googleapis:master Dec 17, 2015
@dhermes dhermes deleted the bigtable-row-filter branch December 17, 2015 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants