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.

Sign up for GitHub

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: Separate row types to remove confusion around return types of row.commit #8662

Merged
merged 4 commits into from
Jul 26, 2019
Merged

Bigtable: Separate row types to remove confusion around return types of row.commit #8662

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a3e72e6
Bigtable: Converging return types of row.commit
Jul 12, 2019
ecc15f1
Merge branch 'master' into converging_row_commit_types
Jul 18, 2019
dc3081d
Fixed snippet tests
Jul 19, 2019
20fcf8e
Variable rename
Jul 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions bigtable/docs/snippets_table.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,89 @@ def test_bigtable_table_row():
table.truncate(timeout=300)


def test_bigtable_table_append_row():
# [START bigtable_table_append_row]
tseaver marked this conversation as resolved.
Show resolved Hide resolved
from google.cloud.bigtable import Client

client = Client(admin=True)
instance = client.instance(INSTANCE_ID)
table = instance.table(TABLE_ID)

row_keys = [b"row_key_1", b"row_key_2"]
row1_obj = table.append_row(row_keys[0])
row2_obj = table.append_row(row_keys[1])
# [END bigtable_table_append_row]

row1_obj.append_cell_value(COLUMN_FAMILY_ID, COL_NAME1, CELL_VAL1)
row1_obj.commit()
row2_obj.append_cell_value(COLUMN_FAMILY_ID, COL_NAME1, CELL_VAL1)
row2_obj.commit()

actual_rows_keys = []
IlyaFaer marked this conversation as resolved.
Show resolved Hide resolved
for row in table.read_rows():
actual_rows_keys.append(row.row_key)

assert actual_rows_keys == row_keys

table.truncate(timeout=300)


def test_bigtable_table_direct_row():
# [START bigtable_table_direct_row]
from google.cloud.bigtable import Client

client = Client(admin=True)
instance = client.instance(INSTANCE_ID)
table = instance.table(TABLE_ID)

row_keys = [b"row_key_1", b"row_key_2"]
row1_obj = table.direct_row(row_keys[0])
row2_obj = table.direct_row(row_keys[1])
# [END bigtable_table_direct_row]

row1_obj.set_cell(COLUMN_FAMILY_ID, COL_NAME1, CELL_VAL1)
row1_obj.commit()
row2_obj.set_cell(COLUMN_FAMILY_ID, COL_NAME1, CELL_VAL1)
row2_obj.commit()

actual_rows_keys = []
IlyaFaer marked this conversation as resolved.
Show resolved Hide resolved
for row in table.read_rows():
actual_rows_keys.append(row.row_key)

assert actual_rows_keys == row_keys

table.truncate(timeout=300)


def test_bigtable_table_conditional_row():
# [START bigtable_table_conditional_row]
from google.cloud.bigtable import Client
from google.cloud.bigtable.row_filters import PassAllFilter

client = Client(admin=True)
instance = client.instance(INSTANCE_ID)
table = instance.table(TABLE_ID)

row_keys = [b"row_key_1", b"row_key_2"]
filter_ = PassAllFilter(True)
row1_obj = table.conditional_row(row_keys[0], filter_=filter_)
row2_obj = table.conditional_row(row_keys[1], filter_=filter_)
# [END bigtable_table_conditional_row]

row1_obj.set_cell(COLUMN_FAMILY_ID, COL_NAME1, CELL_VAL1, state=False)
Copy link

Choose a reason for hiding this comment

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

@crwilcox I feel like instead of state, we could do something like onMatch=False or onMatch=True

Copy link
Contributor

Choose a reason for hiding this comment

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

@billyjacobson that seems reasonable to me. Does that resemble something we do in other languages? I agree state isn't a great name, but it would be good if we didn't diverge from a cross-language pattern if there is one.

Copy link

Choose a reason for hiding this comment

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

Node calls it onMatch and onNoMatch.
The RPCs call it true_mutations and false_mutations
Go uses mtrue, mfalse
PHP trueMutations, falseMutations
Ruby true_mutations, false_mutations

So probably true and false mutations would be most consistent

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't just rename the state parameter: it would be a breaking change.

Copy link

Choose a reason for hiding this comment

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

Ok

row1_obj.commit()
row2_obj.set_cell(COLUMN_FAMILY_ID, COL_NAME1, CELL_VAL1, state=False)
row2_obj.commit()

actual_rows_keys = []
IlyaFaer marked this conversation as resolved.
Show resolved Hide resolved
for row in table.read_rows():
actual_rows_keys.append(row.row_key)

assert actual_rows_keys == row_keys

table.truncate(timeout=300)


def test_bigtable_column_family_name():
# [START bigtable_column_family_name]
from google.cloud.bigtable import Client
Expand Down
62 changes: 62 additions & 0 deletions bigtable/google/cloud/bigtable/table.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,14 @@ def row(self, row_key, filter_=None, append=False):
:raises: :class:`ValueError <exceptions.ValueError>` if both
``filter_`` and ``append`` are used.
"""
warnings.warn(
"This method will be deprecated in future versions. Please "
"use Table.append_row(), Table.conditional_row() "
"and Table.direct_row() methods instead.",
PendingDeprecationWarning,
stacklevel=2,
)

if append and filter_ is not None:
raise ValueError("At most one of filter_ and append can be set")
if append:
Expand All @@ -199,6 +207,60 @@ def row(self, row_key, filter_=None, append=False):
else:
return DirectRow(row_key, self)

def append_row(self, row_key):
"""Create a :class:`~google.cloud.bigtable.row.AppendRow` associated with this table.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make sure these comments reflect why you would choose one of the rows over another.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure, I'm getting. You want something like: you read method's docs, and you don't need to read row class'es docs, 'cause method's docs will tell you everything you need? So classes should be described a little in method's docs?


For example:

.. literalinclude:: snippets_table.py
:start-after: [START bigtable_table_append_row]
:end-before: [END bigtable_table_append_row]

Args:
row_key (bytes): The key for the row being created.

Returns:
A row owned by this table.
"""
return AppendRow(row_key, self)

def direct_row(self, row_key):
"""Create a :class:`~google.cloud.bigtable.row.DirectRow` associated with this table.

For example:

.. literalinclude:: snippets_table.py
:start-after: [START bigtable_table_direct_row]
:end-before: [END bigtable_table_direct_row]

Args:
row_key (bytes): The key for the row being created.

Returns:
A row owned by this table.
"""
return DirectRow(row_key, self)

def conditional_row(self, row_key, filter_):
"""Create a :class:`~google.cloud.bigtable.row.ConditionalRow` associated with this table.

For example:

.. literalinclude:: snippets_table.py
:start-after: [START bigtable_table_conditional_row]
:end-before: [END bigtable_table_conditional_row]

Args:
row_key (bytes): The key for the row being created.

filter_ (:class:`.RowFilter`): (Optional) Filter to be used for
IlyaFaer marked this conversation as resolved.
Show resolved Hide resolved
conditional mutations. See :class:`.ConditionalRow` for more details.

Returns:
A row owned by this table.
"""
return ConditionalRow(row_key, self, filter_=filter_)

def __eq__(self, other):
if not isinstance(other, self.__class__):
return NotImplemented
Expand Down
69 changes: 43 additions & 26 deletions bigtable/tests/unit/test_table.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,16 +180,19 @@ def test_constructor_wo_admin(self):
self.assertIs(table._instance._client, client)
self.assertEqual(table.name, self.TABLE_NAME)

def test_row_factory_direct(self):
from google.cloud.bigtable.row import DirectRow

credentials = _make_credentials()
def _row_methods_helper(self):
client = self._make_client(
project="project-id", credentials=credentials, admin=True
project="project-id", credentials=_make_credentials(), admin=True
)
instance = client.instance(instance_id=self.INSTANCE_ID)
table = self._make_one(self.TABLE_ID, instance)
row_key = b"row_key"
return table, row_key

def test_row_factory_direct(self):
from google.cloud.bigtable.row import DirectRow

table, row_key = self._row_methods_helper()
row = table.row(row_key)

self.assertIsInstance(row, DirectRow)
Expand All @@ -199,13 +202,7 @@ def test_row_factory_direct(self):
def test_row_factory_conditional(self):
from google.cloud.bigtable.row import ConditionalRow

credentials = _make_credentials()
client = self._make_client(
project="project-id", credentials=credentials, admin=True
)
instance = client.instance(instance_id=self.INSTANCE_ID)
table = self._make_one(self.TABLE_ID, instance)
row_key = b"row_key"
table, row_key = self._row_methods_helper()
filter_ = object()
row = table.row(row_key, filter_=filter_)

Expand All @@ -216,28 +213,48 @@ def test_row_factory_conditional(self):
def test_row_factory_append(self):
from google.cloud.bigtable.row import AppendRow

credentials = _make_credentials()
client = self._make_client(
project="project-id", credentials=credentials, admin=True
)
instance = client.instance(instance_id=self.INSTANCE_ID)
table = self._make_one(self.TABLE_ID, instance)
row_key = b"row_key"
table, row_key = self._row_methods_helper()
row = table.row(row_key, append=True)

self.assertIsInstance(row, AppendRow)
self.assertEqual(row._row_key, row_key)
self.assertEqual(row._table, table)

def test_direct_row(self):
from google.cloud.bigtable.row import DirectRow

table, row_key = self._row_methods_helper()
row = table.direct_row(row_key)

self.assertIsInstance(row, DirectRow)
self.assertEqual(row._row_key, row_key)
self.assertEqual(row._table, table)

def test_conditional_row(self):
from google.cloud.bigtable.row import ConditionalRow

table, row_key = self._row_methods_helper()
filter_ = object()
row = table.conditional_row(row_key, filter_=filter_)

self.assertIsInstance(row, ConditionalRow)
self.assertEqual(row._row_key, row_key)
self.assertEqual(row._table, table)

def test_append_row(self):
from google.cloud.bigtable.row import AppendRow

table, row_key = self._row_methods_helper()
row = table.append_row(row_key)

self.assertIsInstance(row, AppendRow)
self.assertEqual(row._row_key, row_key)
self.assertEqual(row._table, table)

def test_row_factory_failure(self):
credentials = _make_credentials()
client = self._make_client(
project="project-id", credentials=credentials, admin=True
)
instance = client.instance(instance_id=self.INSTANCE_ID)
table = self._make_one(self.TABLE_ID, instance)
table, row_key = self._row_methods_helper()
with self.assertRaises(ValueError):
table.row(b"row_key", filter_=object(), append=True)
table.row(row_key, filter_=object(), append=True)

def test___eq__(self):
credentials = _make_credentials()
Expand Down