I'm curious, could you make len(schema_object) work? I guess this is to allow None as schema now, too? If table.schema could indeed return None now, that might be considered a breaking change, depending on how strict we want to be about types.

Either way, folks might have to change their code to deal with missing schema, so something to consider. Maybe we return an object of len(...) == 0 for now and have a warning that it will be changed to None in future?

I dislike the idiom of schema-less tables, but it is indeed something we've seen people do intentionally in the wild. My recollection is that it was used historically as a signal for some workflow state (e.g. some kind of is-table-present check).

I was under the impression that Table.schema can currently be set to None. It appears that we allow for that in the existing code:

https://github.com/googleapis/python-bigquery/blob/d4d39acb8574f0d06d4e490b859e5fe6b57d0d9e/google/cloud/bigquery/table.py#L458C1-L466C84

@schema.setter
def schema(self, value):
    api_field = self._PROPERTY_TO_API_FIELD["schema"]
    if value is None:
        self._properties[api_field] = None
    else:
        value = _to_schema_fields(value)
        self._properties[api_field] = {"fields": _build_schema_resource(value)}

I switched it back to the old check.

Same here. Let's be careful about breaking changes regarding table.schema allowing None.

I also switched this back to the old check.

Flagging with a comment just so we don't lose track of this TODO.

Done.

Lol, oops! 😆 Could be worth a separate cleanup PR to have something small and easy to approve.

Removed.

Why private? I guess because we're trying to emulate a list and want to support mutation that way?

Would be worth an explanation.

You are correct, the idea behind having it be private was to try and emulate a list.
I can add a note to that effect when I add docstrings.

Presumably we want to return a Schema object now.

Aside: Is _parse_schema_resource dead code? I assume it still returns a list of SchemaField?

_parse_schema_resource still returns a list of SchemaFields for backwards compatibility.

Where possible I -tried- to stay consist about what the user provides versus gets back: if someone handed the library a list of SchemaFields (i.e. cause that is what they are used to) then we maintained that state. I tried to avoid randomly converting lists into Schema objects to avoid user surprise.

It might be a bit wasteful to make deepcopy here and in from_api_repr. Indeed it's safer, but could add a lot of overhead. IIRC we actually removed some deepcopy calls from SchemaField because it was slowing down customers who build dynamic schemas in their code.

See: #6 and #26

Same here. May be better to omit the deepcopy.

Could we return an "empty" Schema here for consistency?

@tswast

Does the following influence your thoughts on this comment?

Historically, we have returned an empty list.
An existing user would still expect a list.
A new user should probably not see an empty Schema object, since they are only needed if the user is gonna try to access an external data source (where they have to identify the foreign_type_info).

My general principal, focused on backwards compatibility is:

• avoid as many changes to what the code did before, unless necessary
• as much as possible, force the choice to move to Schema to be an explicit decision by any user (especially since external data sources are likely to be the exception, not the rule).

I could go either way on this one. One one hand, using Schema everywhere would make the new Schema class more discoverable and fewer cases to consider when type checking. On the other hand, providing a real list in all cases where Schema is not needed would provide the most backwards compatible option.

On further consideration, I agree with your assessment that avoiding breaking existing users is preferable. +1 on empty list in this case.

@tswast

this is a related concept.
Right now, if someone gives me a Schema object, I have been storing that in _properties and I see your point about avoiding that.

That complicates getting a Schema object back out, if/when you need it.

What are your thoughts on the following approach? (I seem to recall you mention keeping objects in a separate place and ensuring that _properties is a view into the object)

    @schema.setter
    def schema(self, value):
        api_field = self._PROPERTY_TO_API_FIELD["schema"]

        if value is None:
            self._properties[api_field] = None
        elif isinstance(value, Schema):
            # store original as selt.<original_schema_object> # TODO find a better name
            self.<original_schema_object> = value
            self.properties[api_field] = self.<original_schema_object>.to_api_repr()
        else:
            value = _to_schema_fields(value)
            self._properties[api_field] = {"fields": _build_schema_resource(value)}


    @property
    def schema(self):
        # if user stored a Schema object, short circuit and return early.
        <original_schema_object> = self.get(<original_schema_object>, None)
        if isinstance(original_schema_object, Schema):
            return self.<original_schema_object>

        # if user did not store a Schema object, behave as normal
        prop = self._properties.get(self._PROPERTY_TO_API_FIELD["schema"])
        if not prop: # either None OR empty list  
            return []
        
        return _parse_schema_resource(prop)

My worry with self.<original_schema_object> = value is that it can get out of sync with the values stored in _properties, especially since Schema supports in-place mutation.

Also, there are other cases to consider besides "someone gave us a Schema object". Importantly, what if someone calls get_table("proj.dataset.some_table_with_pangea_features_enabled")? IMO this case is one where we should return a Schema object.

Alternatively, we introduce separate properties for the pangea fields to Table and forget all this Schema class business. Something like Table.schema_foreign_type_info could work. I believe we already do similar for certain statistics on job, for example. The only gotcha here is that we'd need to make sure we merge these two properties (schema and schema_foreign_type_info) rather than replace the whole _properties["schema"] when one sets either of these.

Why _fields. It's fields in the REST API. Also, this should convert to the list to API representation. https://cloud.google.com/bigquery/docs/reference/rest/v2/tables#TableSchema

Changed _fields to fields. Changed tests to match.

If we get a Schema object, the Schema object is responsible for all conversions. I don't think we need to make a copy of it or anything.

Returning the original object should be sufficient.

This also future-proofs us a bit. In case any additional properties are added to Schema, we won't have to update this code.

We don't need this check if we get a Schema object. Let's move that above.

Note: I just return schema if given a Schema. See my comment below.

Alternatively, we could call return Schema.from_api_repr(schema.to_api_repr()), but I don't see much reason to do so.

Please avoid storing non-REST API representation in _properties. Given the usage of UserList, I think just a regular instance variable would be all that's needed.

Unresolving. I don't think _properties for "fields" makes sense in the context of UserList, because we want to support all sorts of mutations and such. For that, let's do something like this instead:

And remove the data and _fields properties. Just a data instance variable is needed. That also has the benefit of not documenting data, which isn't really part of our public API.

This would have to be updated if we remove "fields" from _properties, as I suggest above.

Also, per go/pystyle#truefalse-evaluations-decision it is preferred to use the falsey nature of empty list if possible.

This seems wrong. We should allow people to insert Mapping / dict into their Schema / UserList objects. Even if no remaining field is a SchemaField, we should still return something.

I would expect something more like this:

I'd also like to see some tests where the user replaces all fields with the REST representation of different fields. Something like:

def test_schema_to_api_repr_with_rest_entries():
    schema = Schema(fields=[SchemaField(), ....])
    del schema[:]
    rest_fields = [{"name": "...", "type": "..."}, ...]
    schema.extend(rest_fields)
    got = schema.to_api_repr()
    assert got = {"fields": rest_fields}

Edit: Might be good to have another test such as above but with mixing and matching REST and SchemaField objects. Perhaps by not deleting all entries but instead inserting some REST fields.

With using UserList, you will need to pull out fields and pass them into the constructor.

This makes sure .data has all SchemaField objects while preserving the future-proof ability to add _properties (other than fields).

This comment still stands. We shouldn't have Schema objects in _properties. Instead, check for the existence of any keys besides "fields" to determine if Schema or list should be used.