Discussing current `ex.Table.insert()` behaviour

garysassano commented 1 year ago

Feature Spec

I think maybe Table alone is a too ambiguous term. Currently you can find the following comment for the class definition: Represents a NoSQL database table that can be used to store and query data.

I think calling it something like KVStore would be more clear, but that's for another thread.

What I wanted to discuss is the fact that currently the insert method expects that a certain column is already defined in the table. What does that mean? It means that when you are creating a new cloud.Table you are effectively forced to provide a schema for your entire table, thus making it similar to a relational db table, which is the total opposite of a NoSQL db table/KV store.

So what happens if you try to insert a row that has an attribute/column which wasn't defined during the table creation? Error: "key" is not a valid column in the table.

If we agree that cloud.Table gets translated to DynamoDB::Table in AWS, then we should just enforce the definition of the attribute type of the partition key during the table creation, and not the whole schema, because that's the opposite of how a KV store works.

Use Cases

Make insert method consistent with what a cloud.Table actually represents.

Implementation Notes

No response

Component

No response

Community Notes

Please vote by adding a 👍 reaction to the issue to help us prioritize.
If you are interested to work on this issue, please leave a comment.

MarkMcCulloh commented 1 year ago

+1 that the current Table is probably too ambitious right now, and the current abstraction isn't correct/sufficient.

I think the closest thing the current abstraction was targeting is a DocumentStore, where a strict predetermined schema is not needed (and mostly unwanted). I think we should rename the Table to DocumentStore (or just Document?) and align it closer to how this issue describes (e.g. insertion allows arbitrary values).

I also think this is distinct from a KeyValueStore, which should have a simpler API. For instance, you don't really a need "query" for KeyValueStores, but rather a lookup.

garysassano commented 1 year ago

@MarkMcCulloh The way cloud.Table was working before the recent change to the insert method was totally fine in my opinion. You shouldn't enforce any schema when inserting a new item, the only mandatory attribute in your Json object should be the partition key that you specified during the table creation and that's it. You should be free to add an item with 3 keys, another with 5 keys, etc. since that's how DynamoDB works as well.

I agree that we should make a clear distinction between a key-value database (DynamoDB) and a document-oriented database (DocumentDB) and have two separate classes for them.

I think the current behaviour of cloud.Table represents neither a KV store nor a document-oriented db.

ekeren commented 1 year ago

I would like to offer a different approach: holding off abstracting the DB (for now), I see a couple of advantages:

People that are familiar with dynamoDB will look for how to use it in wing, the abstraction just makes it harder to find
The API will be straightforward, we will be able to move faster with implementing these
Most people don't believe (rightfully so) that you can abstract a DB
I am more curious to see Wing having AtomicMap<Json/ConcreteStruct>, AtomicArray<Json/ConcreteStruct> instead of KeyValueStore, Table (AtomicNumber instead of Counter)

I would also suggest to move this class (and redis) to a bring db namespace, where we can start experimenting with these resources

bring db;
new db.Redis();        // OSS on docker   | elasticCache
new db.DynamoDb();     // dynamodb-local  | DynamoDb
new db.PostgressSql(); // OSS on docker   | Aurora-postgress
new db.MySql();        // OSS on docker   | Aurora-SQL

github-actions[bot] commented 1 year ago

Hi,

This issue hasn't seen activity in 60 days. Therefore, we are marking this issue as stale for now. It will be closed after 7 days. Feel free to re-open this issue when there's an update or relevant information to be added. Thanks!

garysassano commented 1 year ago

Keep.

github-actions[bot] commented 6 months ago

Hi,

This issue hasn't seen activity in 90 days. Therefore, we are marking this issue as stale for now. It will be closed after 7 days. Feel free to re-open this issue when there's an update or relevant information to be added. Thanks!

github-actions[bot] commented 3 months ago

Hi,

This issue hasn't seen activity in 90 days. Therefore, we are marking this issue as stale for now. It will be closed after 7 days. Feel free to re-open this issue when there's an update or relevant information to be added. Thanks!

winglang / wing