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

Working on API updates for the Store & Models #2

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

mvoorberg
Copy link
Contributor

...likely needs more work, but it's coming along.

On my wishlist:

  • Select column lists
  • Pagination w/ skip & limit
  • Ordering
  • Partial update
  • Update to NULL
  • Select where ISNULL
  • ...then IN list support

@aakash-rajur
Copy link
Owner

aakash-rajur commented Feb 11, 2024

w.r.t your store design

insert

  1. existing Insert is variadic and hence supports both InsertOne and InsertBulk.
  2. if your use case is to count the no of inserts, you get inserted records as output of Insert on which you can do length check while also keeping the whole mechanism functional

update

same as insert hence don't see how UpdateOne helps?

find

  1. Find and Update are the most used operations within a transaction in a mutation
  2. if you're operating on multiple rows simultaneously, FindMany and Update (since it's variadic), fits the use case
  3. i don't see how FindOne and FindSingle use cases are not already satisfied with existing design?
  4. FindExactlyOne is usually taken care by unique constraint in db so duplicating this logic in application seems redundant!
  5. FindByPk is also satisfied with Find or FindMany
    id := 1
    name := "John Doe"
    
    // find actor by primary key using `Find`
    actor, err := store.Find(&model.Actor{Id: &id})
    
    // find same actor by name using `Find`
    actor, err := store.Find(&model.Actor{Name: &name});
    
    // find same actor by name using `Find` and vector index search
    actor, err := store.Find(&model.Actor{NameSearch: &name});

delete

  1. i understand DeleteByPk use case but what i've found in retrospect that most existing ORM designs use all columns provided to do a delete to be extra sure that they're deleting the right row
  2. having visited the code again, i feel an improvement over Delete would be to return the deleted row count and accept partial columns to delete many rows at once! (which i'll add shortly!)

count and pagination

  1. there are multiple ways of achieving this, one as you've been suggesting is to have a separate count query which in all fairness is more readable
  2. a more efficient way of achieving this is to use window functions supported by both postgresql and mysql, this avoid an extra io or network request to db and fetches the result in single query.
  3. an even more efficient way would be to use id of last item in a page to fetch next page, this avoids the offset sort performance penalty when your tables are huge
  4. the design you suggest only supports the first use case while a lot of people opt-in for the 2nd approach to begin with and move to 3rd when data grows beyond a threshold

custom

  1. in case none of the generated code satisfy your case, you can write custom queries as specified in the example directory and the tool will generate models for input and output
  2. for the need to have further control, you can directly interact with sqlx as this tool only generates code and is not directly involved!

now if you still feel comfortable with the design you're suggesting i highly recommend you to checkout sqlboiler, it has all the traditional use case you're looking for and lot more than you can imagine

highly appreciate your previous PR for my obvious misses!

@mvoorberg
Copy link
Contributor Author

mvoorberg commented Feb 11, 2024 via email

@aakash-rajur
Copy link
Owner

aakash-rajur commented Feb 12, 2024

ok, can we start with an issue that catalogues all the use case you think are not covered or you think is less accessible?
for ex, bulk insert in a single round trip seems to a very valid use case.

we're jumping to solutions before understanding problem(s), what do you think?

@mvoorberg
Copy link
Contributor Author

mvoorberg commented Feb 12, 2024 via email

@aakash-rajur
Copy link
Owner

aakash-rajur commented Feb 12, 2024

can you create an issue? we can move the discussion there, discuss strategies and solutions. Then we can create a PR addressing this issue

i don't mind creating it but I want it to have you as the author and your idea and extension

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants