Feature Request: Add `FindByColumnList` methods for MySQL

HurSungYun commented 1 year ago

Background

Queries using Where column IN (?) conditions are a common use case in real world.

Therefore, it might be useful for users to support basic FindByColumnList method in Recorder interface.

API Design

When db schema below is given, (copied from README)

message User {
  option (taehoio.ddl.protobuf.v1.datastore_type) = DATASTORE_TYPE_MYSQL;

  int64 id = 1 [(taehoio.ddl.protobuf.v1.key) = true];

  google.protobuf.Timestamp created_at = 2;
  google.protobuf.Timestamp updated_at = 3;
  google.protobuf.Timestamp deleted_at = 4;

  string password_hash = 5;
  string full_name = 6;
  string email = 7 [(taehoio.ddl.protobuf.v1.index) = "name=idx_email"];
}

Generate FindByEmailList method as idx_email exists.

type UserRecorder interface {
    ...
    FindOneByEmail(ctx context.Context, db *sql.DB, email interface{}) (*User, error)
    FindByEmailList(ctx context.Context, db *sql.DB, emailList []interface{}) ([]*User, error) // TO-BE
    FindByEmail(ctx context.Context, db *sql.DB, email interface{}, paginationOpts ...PaginationOption) ([]*User, error)
}

Input & Output

Assume stored user data is below.

id | email
1 | a@abc.com
2 | b@abc.com
3 | c@abc.com

res, _ := userRecorder.FindByEmailList(ctx, db, []string{"a@abc.com", "c@abc.com"})
fmt.Println(res) // output: {{1,"a@abc.com"},{3,"c@abc.com"}}

Considerations

FindByColumnList methods should be generated only if single-column indexes are given.
- Supporting WHERE IN queries based on composite indexes are not simple and its interface can be confused to many users.
Identical result can be achieved calling multiple FindByColumn in parallel, so FindByColumnList methods are not must-have feature now.

xissy commented 1 year ago

Thank you for sharing your valuable suggestion. I understand the appeal of the WHERE IN interface and appreciate your thoughtfulness. However, I am currently considering the fact that this concept is commonly supported in relational databases, while some no-sql datastores may not have full support for it.

May I suggest that we discuss this further and weigh the pros and cons to make an informed decision? I look forward to hearing your thoughts on this matter.

HurSungYun commented 1 year ago

May I suggest that we discuss this further and weigh the pros and cons to make an informed decision?

sure thing. 😉

I am currently considering the fact that this concept is commonly supported in relational databases, while some no-sql datastores may not have full support for it.

I totally agree with you. WHERE IN queries are only for relational databases.

However, I wonder that whether Recorder interfaces would be storage-agnostic or not. For instance, one of Recorder interface in testdata is like below.

type UserCheckinRecorder interface {
    Get(ctx context.Context, db *sql.DB, id int64) (*UserCheckin, error)
    List(ctx context.Context, db *sql.DB, paginationOpts ...PaginationOption) ([]*UserCheckin, error)
    FindByIDs(ctx context.Context, db *sql.DB, ids []int64) ([]*UserCheckin, error)
    Save(ctx context.Context, db *sql.DB, message *UserCheckin) error
    SaveTx(ctx context.Context, tx *sql.Tx, message *UserCheckin) error
    Delete(ctx context.Context, db *sql.DB, id int64) error
    DeleteTx(ctx context.Context, tx *sql.Tx, id int64) error
    FindOneByCountryCodeAndUserIdTypeAndUserIdAndMeasuredAt(ctx context.Context, db *sql.DB, countryCode interface{}, userIdType interface{}, userId interface{}, measuredAtStartTime interface{}, measuredAtEndTime interface{}) (*UserCheckin, error)
    FindByCountryCodeAndUserIdTypeAndUserIdAndMeasuredAt(ctx context.Context, db *sql.DB, countryCode interface{}, userIdType interface{}, userId interface{}, measuredAtStartTime interface{}, measuredAtEndTime interface{}, paginationOpts ...PaginationOption) ([]*UserCheckin, error)
    DeleteByCountryCodeAndUserIdTypeAndUserIdAndMeasuredAt(ctx context.Context, db *sql.DB, countryCode interface{}, userIdType interface{}, userId interface{}, measuredAtStartTime interface{}, measuredAtEndTime interface{}) error
}

(In your plan) Will FindByIDs method always be generated with all types of databases (including DynamoDB)? Actually, FindByIDs method already use WHERE IN queries internally and I believe those types of methods are only supported in relational databases as well. Furthermore, same method might be requested in future for users who use DynamoDB.

If the plan is Recorder interfaces will be storage-agnostic, I prefer not to support WHERE IN queries heavily. If not, I think it's worth to discuss how many use cases for Recorder interfaces to cover.

I believe it's quite common to use WHERE IN queries in relational database world, but I doubt we should recommend to use these queries in protoc-gen-go-ddl. If the roadmap is supporting NoSQL-style simple queries for performance issues, I think it would be better not to support these types of queries. (Although it's a difficult concept to adjust for relational databases)

I want to ask that

Recorder interface will be storage-agnostic or different method will be generated by database systems

It can be a good first question to start a discussion. How do you think about this statement?

xissy commented 1 year ago

@HurSungYun Thank you for the helpful summary, it appears to be a great starting point for resolving the issue. I would like to ask if you would be willing to be a maintainer of this project and make proper decisions. Regarding your question about whether the recorder interface should be storage-agnostic or not, I am inclined to trust your opinions and plans since I know that you have thoughtfully considered many aspects of protoc-gen-go-ddl. So, what is your decision and rationale for it? I would be happy to collaborate and work together.

HurSungYun commented 1 year ago

@xissy

I would like to ask if you would be willing to be a maintainer of this project and make proper decisions.

Yes, absolutely. It would be awesome to be a maintainer of this project. I believe protoc-gen-go-ddl might be able to improve work process of developers around the world. In addition, it's always happy to work with you.

So, what is your decision and rationale for it?

My decision and opinion is, storage-agnostic is not must-have concept. Storage-agnostic concept can be useful in two scenarios.

Storage Migration
Abstraction is heavily used in codebase

In Storage Migration scenario, most cases it migrates data from relational databases to NoSQL databases. In this case, access patterns, data modeling, indexes and all the other stuff are changed. So, we cannot just change database type in most cases and should design everything again. Thus, storage-agnostic is might not be helpful.

In Abstraction scenario, philosophy of Golang is not well-suited for heavy abstraction.

Both scenarios, storage-agnostic concept might not be helpful. Therefore, I want to choose usability over storage-agnostic.

It does not mean we must support WHERE IN queries anyway, but my reasoning is above.

Lastly, protoc-gen-go-ddl is not ORM. I believe we must support some kind of general querying methods in future, but not the storage-agnostic way. Only supporting NoSQL-style simple queries (I mean Get and Set methods) can be put on the discussion table, but It does not have to be same for every databases.

HurSungYun commented 1 year ago

Overall, I think enhancement of Recorder interface is not high priority in this project now. I am sure we can find a good solution after supporting several database types.

xissy commented 1 year ago

@HurSungYun Good. As you may know, the only non-negotiable aspect for protoc-gen-go-ddl, in my opinion, is the indiscriminate use of JOIN operations, which can make it significantly harder to decompose legacies later.

Let me add you as a codeowner and administrator of this repository. I would be delighted to have a meal with you soon to discuss the details. :)

HurSungYun commented 1 year ago

@xissy Thanks! I've accepted your invitation. Feel free to contact me.

taehoio / protoc-gen-go-ddl