stephenafamo/bob

Make query condition for single-column primary key simpler

inoc603 opened this issue · 4 comments

The current implementation for DeleteAll and UpdateAll uses tuple comparison (I'm not sure what's the right name for it) as primary key condition. So even if a table has a single column primary key, we would still generate queries like this:

models.VideoSlice{{ID: 1}, {ID: 2}, {ID: 3}}.DeleteAll(ctx, bob.Debug(db))
DELETE FROM `videos` AS `videos`
WHERE ((`id`) IN ((?), (?), (?)))

I'm working with a project that uses some order version of polardb, which does not seem to support conditions like WHERE (id) in ((?), (?)). While I'm not asking bob to support every mysql variation out there, maybe we can make the query simpler for tables with single-column primary key?

DELETE FROM `videos` AS `videos`
WHERE (`id` IN (?, ?, ?))

I think all we need is something like this in methods like DeleteMany:

	if len(pks) == 1 {
		pkValues := make([]bob.Expression, len(pkVals))
		for i, pair := range pkVals {
			pkValues[i] = pair[0]
		}

		q.Apply(dm.Where(
			Quote(pks[0]).In(pkValues...),
		))
	} else {
		pkPairs := make([]bob.Expression, len(pkVals))
		for i, pair := range pkVals {
			pkPairs[i] = Group(pair...)
		}

		pkGroup := make([]bob.Expression, len(pks))
		for i, pk := range pks {
			pkGroup[i] = Quote(pk)
		}

		q.Apply(dm.Where(
			Group(pkGroup...).In(pkPairs...),
		))
	}

Is there anything else we need to consider? I'm happy to create a PR for it.

I'm conflicted about what to do.

On one hand, I am quite hesitant to include a change in Bob to support older versions of DB engines.
As an example, the MySQL dialect in Bob is strictly for v8.0+ since 5.7 is almost at the EOL.

On the other hand, this seems like a relatively minor change to include.

The feeling's mutual. Maybe we can put aside the 'supporting old DB engines' part, and think of this as generating simpler query for the majority of schemas which have single-column primary key.

Please see #89 for my attempts on this. I'm not sure where should I add tests for this though.

In the refactoring, it seems I have found a way to solve this without too much hassle.

Take a look at the refactor-table-methods branch and try it out.

There are A LOT of breaking changes, and a few things do not fully work yet, that's why #94 is only a draft PR at the moment

Implemented in #94