Additional functions to create filters. #89

coryan · 2017-12-11T22:10:29Z

This is another step to fix #30. It introduces more
functions to create filters, though these are less
common ones.

mbrukman · 2017-12-11T22:13:27Z

bigtable/client/filters.h

+   *
+   * TODO(#84) - document what is the effect of n <= 0
+   */
+  static Filter Latest(std::int32_t n) {


Can we use an unsigned int here, or is int32 the correct type to pass down the call stack?

The rest of the stack expects an int32.

mbrukman · 2017-12-11T22:14:10Z

bigtable/client/filters.h

+   * TODO(#84) - document what is the effect of n <= 0
   */
-  static Filter CellsRowLimit(int n) {
+  static Filter CellsRowLimit(std::int32_t n) {


Same comment here re: unsigned, if possible.

The rest of the stack expects an int32.

mbrukman · 2017-12-11T22:14:16Z

bigtable/client/filters.h

+   * TODO(#84) - document what is the effect of n <= 0
   */
-  static Filter CellsRowOffset(int n) {
+  static Filter CellsRowOffset(std::int32_t n) {


Same comment here re: unsigned, if possible.

The rest of the stack expects an int32.

mbrukman · 2017-12-11T22:15:52Z

bigtable/client/filters.h

+   * TODO(#84) - decide what happens if the probability is out of range.
+   *
+   * @param probability the probability that any row will be selected.  It
+   *     must be in the [0.0, 1.0] range.


[...] must be in the range [0.0, 1.0].

but you can also simplify it to:

[...] must be in the range [0, 1].

Did the first, I like making it explicit that is a floating point range, but will change if you feel strongly about it.

Floating point range SGTM.

mbrukman · 2017-12-11T22:16:28Z

bigtable/client/filters.h

+   * different ranges the following functions are available.
+   */
+  /**
+   * Return a filter that accepts values in the [@p start, @p end) range.


[...] values in the range [@p start, @p end).

mbrukman · 2017-12-11T22:18:30Z

bigtable/client/filters.h

+  }
+
+  /**
+   * Return a filter that accepts columns in the (@p start, @p end) range


Same as above.

mbrukman · 2017-12-11T22:18:50Z

bigtable/client/filters.h

+   * Each value accepted by previous filters in modified to include the @p
+   * label.
+   *
+   * @note Currently it is not possible to apply more than one label in a


Currently, it is ...

mbrukman · 2017-12-11T22:19:12Z

bigtable/client/filters.h

+   *     filter expression, that is, a chain can only contain a single
+   *     ApplyLabelTransformer() filter.  This limitation may be lifted in
+   *     the future.  It is possible to have multiple ApplyLabelTransformer
+   *     filters in a Union() filter, though in this case each copy of a cell


, though in this case, each [...]

mbrukman · 2017-12-11T22:20:05Z

bigtable/client/filters.h

+   *
+   * @param label the label applied to each cell.  The labels must be at most 15
+   *     characters long, and must match the `[a-z0-9\\-]` pattern.
+   * TODO(#84) - change this if we decide to validate inputs in the client side


Will this TODO be automatically included in the documentation on the previous line for @param label? Did you want that to happen? Do we need a blank line between the arg doc comment and the TODO to dissociate them?

Same comment elsewhere, I've seen this pattern a few times.

Did not want that, thanks for catching it. Fixed, and looked for the pattern elsewhere too.

mbrukman · 2017-12-11T22:23:04Z

bigtable/client/filters.h

+   *
+   * TODO(#84) - document what happens if end < start
+   */
+  static Filter ColumnRangeLeftOpen(std::string column_family,


@sduskis, @garye, @DanielMahu — do we actually support column range queries? I thought we only support row range queries, and column families and column qualifiers need to be specified explicitly, one at a time.

ColumnRangeFilters can be applied just like any other filter:
https://github.com/googleapis/googleapis/blob/master/google/bigtable/v2/data.proto#L390

OK, got it, so column families are discrete (must be specified explicitly), while column qualifiers are continuous (can be selected individually, or via a range).

This is another step to fix googleapis#30. It introduces more functions to create filters, though these are less common ones.

coryan · 2017-12-15T14:33:34Z

Ping. @mbrukman I think I addressed the issues you raised. @garye can you take a look?

mbrukman

Minor comments, please feel free to squash + merge after you address them, though the style guide on comments should be added as a TODO somewhere, perhaps (in the style guide PR, maybe?).

mbrukman · 2017-12-17T23:32:35Z

bigtable/client/filters.h


-  /// Create a filter that accepts only the last @a n values.
-  static Filter Latest(int n) {
+  /**


Do we have a pattern for when we use /** ... */ vs. when we use /// comments? Does Doxygen handle both of them? Do they behave differently somehow?

This should be documented in the style guide, and we should be consistent, assuming they are identical.

The style guide proposed in #98 includes guidelines for when to use /** */ vs. /// and yes, Doxygen supports both.

mbrukman · 2017-12-17T23:33:39Z

bigtable/client/filters.h

+   * range.  In most cases applications use [start,end) ranges, and the
+   * ValueRange() and ColumnRange() functions are offered to support the
+   * common case.  For the less common cases where the application needs
+   * different ranges the following functions are available.


s/ranges the/ranges, the/

mbrukman · 2017-12-17T23:33:52Z

bigtable/client/filters.h

+   * @name Less common range filters.
+   *
+   * Cloud Bigtable range filters can include or exclude the limits of the
+   * range.  In most cases applications use [start,end) ranges, and the


s/start,end/start, end/ (add space after comma)

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 11, 2017

coryan requested review from DanielMahu, garye, mbrukman and sduskis December 11, 2017 22:10

mbrukman suggested changes Dec 11, 2017

View reviewed changes

Additional functions to create filters.

904595b

This is another step to fix googleapis#30. It introduces more functions to create filters, though these are less common ones.

coryan force-pushed the fix-issue-30-implement-filters-p2 branch from dbf3fd3 to 904595b Compare December 11, 2017 23:59

Address comments from review.

1fa9268

coryan assigned garye Dec 15, 2017

garye approved these changes Dec 15, 2017

View reviewed changes

mbrukman approved these changes Dec 17, 2017

View reviewed changes

Address comments from review.

29d50a1

coryan merged commit 5a43f75 into googleapis:master Dec 18, 2017

coryan deleted the fix-issue-30-implement-filters-p2 branch December 18, 2017 03:27

Additional functions to create filters. #89

Additional functions to create filters. #89

Uh oh!

Conversation

coryan commented Dec 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coryan Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coryan Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coryan Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coryan commented Dec 15, 2017

Uh oh!

mbrukman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

coryan Dec 11, 2017 •

edited

Loading

coryan Dec 11, 2017 •

edited

Loading

coryan Dec 11, 2017 •

edited

Loading