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

bigtable: Allow microseconds for TimestampRangeFilter (again) #5931

Closed
mmetto opened this issue Apr 22, 2022 · 7 comments
Closed

bigtable: Allow microseconds for TimestampRangeFilter (again) #5931

mmetto opened this issue Apr 22, 2022 · 7 comments
Assignees
Labels
api: bigtable Issues related to the Bigtable API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@mmetto
Copy link

mmetto commented Apr 22, 2022

Is your feature request related to a problem? Please describe.
Could you please remove code rejecting microseconds when defining TimestampRangeFilters?

Describe the solution you'd like

  1. Remove if-block here
  2. Update tests here (remove these 2 lines)

Describe alternatives you've considered
Detect which API version is used (v1 vs. v2) and deny or allow microseconds for TimestampRangeFilter accordingly.

Additional context
It seems v2 APIs/clients use microseconds now when defining TimestampRangeFilters. This breaks most tests we have after upgrading gcp packages to get our dev environments working on Mac M1.


The v2 proto defines TimestampFilter using micros:

# google.bigtable.v2.data:
// Specified a contiguous range of microsecond timestamps.
message TimestampRange {
  // Inclusive lower bound. If left empty, interpreted as 0.
  int64 start_timestamp_micros = 1;

  // Exclusive upper bound. If left empty, interpreted as infinity.
  int64 end_timestamp_micros = 2;
}

and here is Filters.java coming from com.google.cloud.bigtable.data.v2.models package:

        public static final class TimestampRangeFilter
      extends AbstractTimestampRange<TimestampRangeFilter> implements Filter {
    ...
    public RowFilter toProto() {
      com.google.bigtable.v2.TimestampRange.Builder builder =
          com.google.bigtable.v2.TimestampRange.newBuilder();

      switch (getStartBound()) {
        case CLOSED:
          builder.setStartTimestampMicros(getStart());
          break;
        case OPEN:
          builder.setStartTimestampMicros(getStart() + 1);
          break;
        case UNBOUNDED:
          break;
        default:
          throw new IllegalStateException("Unknown start bound: " + getStartBound());
      }
@mmetto mmetto added the triage me I really want to be triaged. label Apr 22, 2022
@product-auto-label product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label Apr 22, 2022
@telpirion telpirion added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed triage me I really want to be triaged. labels Apr 22, 2022
@mmetto
Copy link
Author

mmetto commented May 18, 2022

Hi @telpirion can we get traction on this issue? Thanks in advance!

@mmetto
Copy link
Author

mmetto commented Jun 1, 2022

Hello @telpirion kindly reminder about this issue.

@fredsadaghiani
Copy link

This is affecting my project as well. Thanks for the fix @mmetto!

@mmetto
Copy link
Author

mmetto commented Aug 2, 2022

@telpirion @igorbernstein2 can I get your eyes on the linked PR? This is blocking our testing and requires manual changes to get the tests working properly with newest bigtable client/models locally, while also locking us to use old version of dependencies for our service.
If you're not happy with the changes introduced, please feel free to reach out to me and let's get things worked out.

@icio
Copy link
Contributor

icio commented Aug 10, 2022

This obscure error has caused me some issues too.

@mmetto
Copy link
Author

mmetto commented Aug 22, 2022

@telpirion @igorbernstein2 any chance we can get traction on this issue?

@telpirion
Copy link
Contributor

Closing as the Bigtable service behaves similarly to the emulator.

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. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants