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.

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

Discussing current ex.Table.insert() behaviour #2437

Open
garysassano opened this issue May 8, 2023 · 7 comments
Open

Discussing current ex.Table.insert() behaviour #2437

garysassano opened this issue May 8, 2023 · 7 comments
Labels
✨ enhancement New feature or request needs-discussion Further discussion is needed prior to impl 🎨 sdk SDK

Comments

@garysassano
Copy link
Collaborator

garysassano commented May 8, 2023

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.
@garysassano garysassano added ✨ enhancement New feature or request 🎨 sdk SDK labels May 8, 2023
@MarkMcCulloh
Copy link
Contributor

+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
Copy link
Collaborator Author

@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
Copy link

ekeren commented Jun 8, 2023

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
Copy link

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!

@github-actions github-actions bot added the Stale label Aug 10, 2023
@garysassano
Copy link
Collaborator Author

Keep.

@github-actions github-actions bot removed the Stale label Aug 11, 2023
@staycoolcall911 staycoolcall911 added the needs-discussion Further discussion is needed prior to impl label Oct 4, 2023
@staycoolcall911 staycoolcall911 changed the title Discussing current cloud.Table.insert() behaviour Discussing current ex.Table.insert() behaviour Oct 4, 2023
Copy link

github-actions bot commented Apr 8, 2024

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!

Copy link

github-actions bot commented Jul 8, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request needs-discussion Further discussion is needed prior to impl 🎨 sdk SDK
Projects
Status: 🤝 Backlog - handoff to owners
Development

No branches or pull requests

4 participants