feat: version filters

[kboroszko]

This PR adds support for Bigtable version filters.

---

This change is 

[dopiera]

Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @dopiera and @kboroszko)

---

tensorflow_io/core/kernels/bigtable/bigtable_dataset_kernel.cc, line 326 at r1 (raw file):

filter_resource_

Given my limited understanding of the lifetimes of these object I think I'd be calmer if we kept the filter rather than the reference to the resource.

---

tensorflow_io/core/kernels/bigtable/bigtable_version_filters.h, line 49 at r1 (raw file):

  }

  google::cloud::bigtable::Filter& filter() { return filter_; }

I think we should either return a cref or a value or call this member function in a way indicating that it returns a reference.

BTW, don't we need to care about concurrency here?

---

tensorflow_io/core/kernels/bigtable/bigtable_version_filters.cc, line 48 at r1 (raw file):

      : AbstractBigtableResourceOp<BigtableFilterResource>(ctx) {
    VLOG(1) << "BigtableTimestampRangeFilterOp ctor ";
    OP_REQUIRES_OK(ctx, ctx->GetAttr("start", &start_));

Given that the type is essentially and int, why not make it a bit easier to the reader to figure out what it is and call it start_ts_us or at least start_us to indicate it's a timestamp expressed in microseconds?

---

tensorflow_io/core/kernels/bigtable/bigtable_version_filters.cc, line 57 at r1 (raw file):

  mutable mutex mu_;

You don't need the mutex, do you?

---

tensorflow_io/core/kernels/bigtable/bigtable_version_filters.cc, line 59 at r1 (raw file):

  int64_t start_;
  int64_t end_;

These can be const.

---

tensorflow_io/core/kernels/bigtable/bigtable_version_filters.cc, line 86 at r1 (raw file):

  mutable mutex mu_;

I don't think you need the mutex

[dopiera]

It looks good in general, I left a couple of nitpicks.

Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @dopiera and @kboroszko)

[kboroszko]

Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @dopiera)

---

tensorflow_io/core/kernels/bigtable/bigtable_dataset_kernel.cc, line 326 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

filter_resource_

Given my limited understanding of the lifetimes of these object I think I'd be calmer if we kept the filter rather than the reference to the resource.

Done.

---

tensorflow_io/core/kernels/bigtable/bigtable_version_filters.h, line 49 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

I think we should either return a cref or a value or call this member function in a way indicating that it returns a reference.

BTW, don't we need to care about concurrency here?

We set it in the constructor and never change it afterwards, so I'd say we don't since it's read-only. I changed it to const. How about that?

---

tensorflow_io/core/kernels/bigtable/bigtable_version_filters.cc, line 48 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

Given that the type is essentially and int, why not make it a bit easier to the reader to figure out what it is and call it start_ts_us or at least start_us to indicate it's a timestamp expressed in microseconds?

Done.

---

tensorflow_io/core/kernels/bigtable/bigtable_version_filters.cc, line 57 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

  mutable mutex mu_;

You don't need the mutex, do you?

Done.

---

tensorflow_io/core/kernels/bigtable/bigtable_version_filters.cc, line 59 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

  int64_t start_;
  int64_t end_;

These can be const.

They can't because I must set them in the constructor with ctx->GetAttr(name, ptr_to_value).

---

tensorflow_io/core/kernels/bigtable/bigtable_version_filters.cc, line 86 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

  mutable mutex mu_;

I don't think you need the mutex

Done.

[dopiera]

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @dopiera)