Skip to content

feat(bigtable): add support read row by prefix#3

Draft
HemangChothani wants to merge 14 commits into
masterfrom
bigtable_issue_24
Draft

feat(bigtable): add support read row by prefix#3
HemangChothani wants to merge 14 commits into
masterfrom
bigtable_issue_24

Conversation

@HemangChothani
Copy link
Copy Markdown
Collaborator

Fixes [24]

@HemangChothani HemangChothani requested a review from mf2199 May 7, 2020 12:17
Comment thread docs/snippets_table.py
Comment on lines +708 to +720
read_rows = table.read_rows(row_set=row_set)
expected_row_keys = [
b"row_key_1",
b"row_key_2",
b"row_key_3",
b"row_key_4",
b"row_key_5",
b"row_key_6",
b"row_key_7",
b"row_key_8",
b"row_key_9",
]
found_row_keys = [row.row_key for row in read_rows]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, a more elegant and concise way would be

expected_row_keys = [b"row_key_%x" % i for i in range(1, 10)]
found_row_keys = [row.row_key for row in table.read_rows(row_set=row_set)]

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes , correct but they have declare static before in snippets and system file so ta make consistent i made this static.

@mf2199 mf2199 requested review from IlyaFaer and paul1319 May 7, 2020 17:51

:type row_key_prefix: str
:param row_key_prefix: Add all rows that start with this row key prefix.
Prefix cannot be zero length."""
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect indentation, and the comment doesn't seem to be very understandable.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:param row_key_prefix: Add all rows that start with this row key prefix.
Prefix cannot be zero length."""

end_key = row_key_prefix[:-1] + chr(ord(row_key_prefix[-1]) + 1)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems strange. If you'll pass "row" (as you did in your sample), you'll get "rox". What it should mean?
Sounds to me, in this variable we should have a name of the key on which we'd like to stop fetching data from table.

In Billy's example there is a "#" symbol. After +1 it'll become "$" which is the "end of line" regex symbol. It sound logical, but incrementing the code of the last symbol overall doesn't seem correct to me.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# and $ doesn't work here we need to remove manually , Please see the implementation of prefix in go and node:

Following links of integration test :
Go: https://github.com/googleapis/google-cloud-go/blob/51eb5ee54e44743973ca6a638660fd1067b17365/bigtable/integration_test.go#L673

Node:
https://github.com/googleapis/google-cloud-node/pull/1802/files#diff-5528ad90973774d4a89e43a0462bf388R233


:type row_key_prefix: str
:param row_key_prefix: Add all rows that start with this row key prefix.
:param row_key_prefix: To retrieve all rows that start with this row key prefix.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To retrieve all rows that start with this row key prefix.

The grammar here can be improved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants