Document the order of cells within a row. #146

coryan · 2017-12-26T13:20:34Z

Such as it is (the order is not guaranteed for column families).
This fixes #82.

Such as it is (the order is not guaranteed for column families). This fixes googleapis#82.

garye · 2017-12-26T14:49:26Z

bigtable/client/filters.h

+   * by:
+   * - The column family internal ID, which is not necessarily the
+   *   lexicographical order of the column family names.  Also, the ID of each
+   *   column family may change on each row.


I'm not quite sure how to parse this. Is there a way to rephrase? Overall the description seems accurate, though.

Even though these are only comment changes I am not skipping the CI build because they affect Doxygen.

coryan · 2017-12-26T19:45:39Z

PTAL.

carterpage · 2017-12-28T19:03:17Z

bigtable/client/filters.h

+   * filter apples to the cells within a row, if there are multiple column
+   * families and/or columns in a row the order is:
+   * - All the cells for a column family appear together, but there is no
+   *   guarantee on the order of the column families.  Furthermore, column


Removing the "internal ID" reference is an improvement. It is best to avoid referring to internal mechanics that the user can't see.

mbrukman · 2017-12-28T19:11:39Z

bigtable/client/filters.h

+   * of multiple filters via the `Interleave()` function.  Furthermore, this
+   * filter apples to the cells within a row, if there are multiple column
+   * families and/or columns in a row the order is:
+   * - All the cells for a column family appear together, but there is no


Have you checked the rendering of this in Doxygen output? Doxygen supports Markdown, but most Markdown parsers I've seen require adding a blank line before a bulleted list; otherwise, it doesn't render properly.

Yes, see below.

LGTM, thanks for checking.

mbrukman · 2017-12-28T19:11:56Z

bigtable/client/filters.h

+   * of multiple filters via the `Interleave()` function.  Furthermore, this
+   * filter apples to the cells within a row, if there are multiple column
+   * families and/or columns in a row the order is:
+   * - All the cells for a column family appear together, but there is no


Same comment about rendering as above.

I did, it works. Look at the attached file:

filter-doxygen-render.pdf

Perfect, thanks!

coryan · 2017-12-28T19:22:22Z

PTAL.

mbrukman · 2017-12-28T19:27:21Z

bigtable/client/filters.h

+   * - All the cells for a column family appear together, but there is no
+   *   guarantee on the order of the column families.  Furthermore, column
+   *   families may appear in different orders in different rows.
+   * - Within a column family the cells are ordered by column name, column names


Within a column family, the cells are ordered by column name, where column names are sorted lexicographically.

mbrukman · 2017-12-28T19:27:34Z

bigtable/client/filters.h

+   *   families may appear in different orders in different rows.
+   * - Within a column family the cells are ordered by column name, column names
+   *   are compared lexicographically.
+   * - With a column, the cells appear in descending order by timestamp.


s/With/Within/

mbrukman · 2017-12-28T19:27:46Z

bigtable/client/filters.h

+   *   families may appear in different orders in different rows.
+   * - Within a column family the cells are ordered by column name, column names
+   *   are compared lexicographically.
+   * - With a column, the cells appear in descending order by timestamp.


s/With/Within/

mbrukman · 2017-12-28T19:29:54Z

bigtable/client/filters.h

-   * TODO(#82) - check the documentation around ordering of columns.
+   * Note that cells might be repeated, such as when interleaving the results
+   * of multiple filters via the `Interleave()` function.  Furthermore, this
+   * filter apples to the cells within a row, if there are multiple column


s/row, if/row; if/

mbrukman · 2017-12-28T19:30:06Z

bigtable/client/filters.h

+   * Note that cells might be repeated, such as when interleaving the results
+   * of multiple filters via the `Interleave()` function.  Furthermore, this
+   * filter apples to the cells within a row, if there are multiple column
+   * families and/or columns in a row the order is:


s/row the/row, the/

mbrukman · 2017-12-28T19:31:31Z

bigtable/client/filters.h

+   * - All the cells for a column family appear together, but there is no
+   *   guarantee on the order of the column families.  Furthermore, column
+   *   families may appear in different orders in different rows.
+   * - Within a column family the cells are ordered by column name, column names


Within a column family, the cells are ordered by column name, where column names are sorted lexicographically.

mbrukman · 2017-12-28T19:32:17Z

bigtable/client/filters.h

-   * TODO(#82) - check the documentation around ordering of columns.
+   * Note that cells might be repeated, such as when interleaving the results
+   * of multiple filters via the `Interleave()` function.  Furthermore, this
+   * filter apples to the cells within a row, if there are multiple column


s/row, if/row; if/

Fixed, this is identical to the previous comment, so I just copied it.

mbrukman · 2017-12-28T19:32:49Z

bigtable/client/filters.h

+   * Note that cells might be repeated, such as when interleaving the results
+   * of multiple filters via the `Interleave()` function.  Furthermore, this
+   * filter apples to the cells within a row, if there are multiple column
+   * families and/or columns in a row the order is:


s/row the/row, the/

coryan · 2017-12-28T19:41:26Z

PTAL.
I did not skip the ci build because Doxygen, though I expect no breakage.

mbrukman

LGTM

Document the order of cells within a row.

8aa0e14

Such as it is (the order is not guaranteed for column families). This fixes googleapis#82.

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

coryan requested review from carterpage and garye December 26, 2017 14:47

garye reviewed Dec 26, 2017

View reviewed changes

Rephrase the ordering guarantees per review.

4218e5e

Even though these are only comment changes I am not skipping the CI build because they affect Doxygen.

carterpage reviewed Dec 28, 2017

View reviewed changes

mbrukman reviewed Dec 28, 2017

View reviewed changes

mbrukman suggested changes Dec 28, 2017

View reviewed changes

Address comments from review.

4e8846a

mbrukman approved these changes Dec 28, 2017

View reviewed changes

mbrukman merged commit bfdd4dc into googleapis:master Dec 28, 2017

coryan deleted the document-order-of-cells branch December 28, 2017 20:08

Document the order of cells within a row. #146

Document the order of cells within a row. #146

Uh oh!

Conversation

coryan commented Dec 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coryan commented Dec 26, 2017

Uh oh!

carterpage Dec 28, 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!

coryan commented Dec 28, 2017

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 28, 2017

Uh oh!

mbrukman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

carterpage Dec 28, 2017 •

edited

Loading