BigQuery: Use TableListItem for table listing.

[tswast]

The table list response only includes a subset of all table properties.
This commit adds a new type to document explicitly which properties are
included, but also make it clear that this object should not be used in
place of a full Table object.

Towards #4373 (needs a similar change for dataset listing before closing that issue)

[dhermes]

A few design questions:

1. How many of these @property-s are copy-pasta?
2. I don't think the class name really conveys what it is (a read-only version of Table).
3. Is there a reason you might be opposed to just subclassing Table or just an arbitrary decision to make a new class?
4. Are you opposed to adding a read_only attribute / member to Table?

[tswast]

1. How many of these @property-s are copy-pasta?

Basically all of them. I made a slight change to view_use_legacy_sql to use the type property to check if a table is a view. This is because the query property of the view is not included in the table list response, so the type property is the only way to check if the table is a view when useLegacySql is left at the default value.

2. I don't think the class name really conveys what it is (a read-only version of Table).

It being read-only is more of a side-effect than the primary purpose of the class.

3. Is there a reason you might be opposed to just subclassing Table or just an arbitrary decision to make a new class?

The primary purpose of TableListItem is to indicate that there is a different set of properties available on the list response than a full Table object. Notable properties that aren't there include schema and total rows (#4373).

A sub-class would allow this object to be used in any place a table is allowed, such as update or create requests, which is not something I want people to be doing (because not all properties are present).

4. Are you opposed to adding a read_only attribute / member to Table?

I hadn't really considered it, because it wouldn't solve the problem of schema or num_rows not being populated from a list operation.

[dhermes]

@tswast I'm just trying to avoid code duplication, because the copy-pasta will drift over time.

If you did a subclass, you could over-ride and raise an AttributeError for properties not contained in the response (or you could do it in the Table class by using self._read_only to check).

The same applies to methods like Table.update() and Table.create().

I think having a different class is probably the right move as far as "principle of least astonishment" is concerned. (Or rather, if you returned a Table that was limited, users might be surprised/astonished if they never checked table.read_only.) But I'd strongly suggest making the other class have much less code defined in it / leverage the existing stuff.

[tswast]

There's so little code in these properties, I'm not super concerned about drift.

[dhermes]

There's so little code in these properties, I'm not super concerned about drift.

But I am concerned. Care to sell me a bit on your position?

[tswast]

Most of these properties have implementations like:

return self._properties.get('tableReference', {}).get('datasetId')

These properties directly reflect properties on the resource, with minimal modification.

The reference does have code which could drift.

from google.cloud.bigquery import dataset

dataset_ref = dataset.DatasetReference(self.project, self.dataset_id)
return TableReference(dataset_ref, self.table_id)

I agree this could be shared with the Table class, but the Table class doesn't have this property yet. I'm adding it in #4405. I could update that PR to share the code with the TableListItem class once this one goes in.

You're right that view_use_legacy_sql is code that could drift, too. I think there are probably better ways to share that code than a subclass. I'll make an update to the PR.

[dhermes]

@tswast Though these are simple "schema->data->Python" mappings, some APIs (BQ included) have changed the underlying schema and caught us off guard. With that in mind, "so little code" doesn't inspire much confidence?

Also, I don't want to get lost in this discussion that I think TableListItem is a not great name. Other candidates? ReadOnlyTable?

[tswast]

Discussed in chat. Since Table and "whatever we call the resource from table list" are actually decoupled from each other in the backend, it doesn't make sense to couple them here.

[dhermes]

@tswast OK Do you feel this is merge-able?

[tswast]

Do you feel this is merge-able?

Yeah. Just ran the full nox suite locally to double-check.

[dhermes]

Cool LGTM