Implement Connection.predefine_table_schema (#422)

[shmygol]

Implement Connection.predefine_table_schema, which provides manual populating of cached table schemes (Connection._tables) and helps avoid to request DescribeTable which takes some time.

The method is integrated in TableConnection, which is used in model. It means all models don't send DescribeTable requests before querying the data any more, but Model.describe_table sends the real request every time it's called.

[shmygol]

I didn't do any refactoring to avoid unnecessary big pull requests, but I'd suggest Model._get_schema and Model._get_indexes to return the some keys in camel case instead of underscore to avoid transforming the data over and over again. Example:

        self.test_predefined_schema = {
            "attribute_definitions": [
                {
                    "AttributeName": "ForumName",
                    "AttributeType": "S"
                },
                {
                    "AttributeName": "LastPostDateTime",
                    "AttributeType": "S"
                },
                {
                    "AttributeName": "Subject",
                    "AttributeType": "S"
                }
            ],
            "key_schema": [
                {
                    "AttributeName": "ForumName",
                    "KeyType": "HASH"
                },
                {
                    "AttributeName": "Subject",
                    "KeyType": "RANGE"
                }
            ],
            "global_secondary_indexes": [
                {
                    "IndexName": "LastPostIndex",
                    "KeySchema": [
                        {
                            "AttributeName": "ForumName",
                            "KeyType": "HASH"
                        },
                        {
                            "AttributeName": "LastPostDateTime",
                            "KeyType": "RANGE"
                        }
                    ],
                    "Projection": {
                        "ProjectionType": "KEYS_ONLY"
                    }
                }
            ],
            "local_secondary_indexes": [
                {
                    "IndexName": "LastPostIndex",
                    "KeySchema": [
                        {
                            "AttributeName": "ForumName",
                            "KeyType": "HASH"
                        },
                        {
                            "AttributeName": "LastPostDateTime",
                            "KeyType": "RANGE"
                        }
                    ],
                    "projection": {
                        "ProjectionType": "KEYS_ONLY"
                    }
                }
            ],
        }

I could do it if there are no objections.

[shmygol]

Do anybody has an idea when the PR is going to be reviewed? Thanks!

[ikonst]

Would it be possible to make that DescribeTable call unnecessary? Our model definition already describes the table's keys and indices so we can just use them.

Of course DescribeTable gets that same information from the source of truth, but we don't actually take advantage of that to call out discrepancies between the model definition and the table definition: instead, in those cases PynamoDB fails in confusing ways (e.g. if the table defines a key with a different underlying data type vs. our model definition).

I remember @jpinner-lyft was working on removing that DescribeTable call, but I don't think he ever completed that effort.

[shmygol]

That’s the point of the PR. Connection doesn’t call DescribeTable if the scheme is provided, which is already the case for models. Ilya Konstantinov <notifications@github.com> schrieb am Sa. 4. Apr. 2020 um 22:16:

Would it be possible to make that DescribeTable call unnecessary? Our model definition already describes the table's keys and indices so we can just use them. Of course DescribeTable gets that same information from the source of truth, but we don't actually take advantage of that to call out discrepancies between the model definition and the table definition: instead, in those cases PynamoDB fails in confusing ways (e.g. if the table defines a key with a different underlying data type vs. our model definition). I remember @jpinner-lyft <https://github.com/jpinner-lyft> was working on removing that DescribeTable call, but I don't think he ever completed that effort. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#749 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGFF33SRR6X6VIO2METBGTRK6ITNANCNFSM4KNRTQWQ> .

-- Best Regards, Ilya Shmygol.

[ikonst]

Ah, I missed the fact that you automatically produce a predefined schema in Model, nice.

One thought, though, is that the only reason TableConnection._tables exists is for TableConnection.get_meta_table. Perhaps the concept of storing table schemas could be removed from TableConnection entirely? After all, if in all practical cases it'll be echoing back to the Model class the predefined schema that the Model class supplied it with, perhaps there's no need for this complexity?

[shmygol]

Ah, I missed the fact that you automatically produce a predefined schema in Model, nice.

One thought, though, is that the only reason TableConnection._tables exists is for TableConnection.get_meta_table. Perhaps the concept of storing table schemas could be removed from TableConnection entirely? After all, if in all practical cases it'll be echoing back to the Model class the predefined schema that the Model class supplied it with, perhaps there's no need for this complexity?

@ikonst do you mean avoid caching schemas in a class and store the table schema in an instance variable instead (we still have to store the schema somewhere)? I was afraid to introduce decline of performance for clients who use TableConnection without a model for some reason.

But I find your suggestion reasonable and can do it. Here how I see it: TableConnection.predefine_table_schema becomes an instance method and populate an instance variable TableConnection._table. If table schema is not predefined it's fetched lazily in TableConnection.get_meta_table from DynamoDB, but only for this instance.

Does it make sense to you?

[ikonst]

(Sorry, been very busy lately, will have a look over the weekend)

[ikonst]

@ikonst do you mean avoid caching schemas in a class and store the table schema in an instance variable instead (we still have to store the schema somewhere)? I was afraid to introduce decline of performance for clients who use TableConnection without a model for some reason.

It's hard for me to imagine TableConnection is used by anyone but the library itself, though with enough users there's someone for every obscure use case :)

If table schema is not predefined it's fetched lazily in TableConnection.get_meta_table from DynamoDB, but only for this instance.

What I'm questioning is why the connection/model needs to be aware of the server's table schema. While it's the source of truth, in many other places we act based on the local schema. The schemas can be out of sync -- for example, server thinks HK is a number (N), locally it's a UnicodeAttribute (S). Do we use server's schema or local schema when serializing and deserializing? From last time I read the code, it's a mix of both, which is probably the most confusing and error prone approach :/

The library defines a local schema that's more extensive than the server-side schema -- we define non-key attributes while DynamoDB defines the keys only, and we have some complex data types on top of DynamoDB's built-in ones, so the server's schema is not sufficient to serialize/deserialize a model; thus, we need the local schema. Because of this, I'd rather see PynamoDB use only the local schema in all serialization/deserialization, in which case DescribeTable should be completely optional in normal operation.

I think there's a good function where DescribeTable would be useful, and that's an (imaginary) "Model.validate_schema" method that would ensure that the local schema overlaps with the server's one (e.g. that we don't mistype indexes, which can create some really confusing error messages... from experience).