From 69af496038c679cebf52adb3e783d38d8d2e380b Mon Sep 17 00:00:00 2001 From: Dylan Boltz Date: Mon, 10 Jun 2019 15:07:00 +0900 Subject: [PATCH 1/6] Changed the way records are counted --- finders.go | 8 ++++---- model.go | 5 +++-- sql_builder.go | 4 +++- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/finders.go b/finders.go index 13a6b609f..2b1e6f5da 100644 --- a/finders.go +++ b/finders.go @@ -332,7 +332,7 @@ func (q Query) CountByField(model interface{}, field string) (int, error) { tmpQuery.Paginator = nil tmpQuery.orderClauses = clauses{} tmpQuery.limitResults = 0 - query, args := tmpQuery.ToSQL(&Model{Value: model}) + query, args := tmpQuery.ToSQL(&Model{Value: model, ignoreTableName: true}, "COUNT(*)") //when query contains custom selected fields / executed using RawQuery, // sql may already contains limit and offset @@ -344,9 +344,9 @@ func (q Query) CountByField(model interface{}, field string) (int, error) { query = query[0 : len(query)-len(foundLimit)] } - countQuery := fmt.Sprintf("SELECT COUNT(%s) AS row_count FROM (%s) a", field, query) - log(logging.SQL, countQuery, args...) - return q.Connection.Store.Get(res, countQuery, args...) + // countQuery := fmt.Sprintf("SELECT COUNT(%s) AS row_count FROM (%s) a", field, query) + log(logging.SQL, query, args...) + return q.Connection.Store.Get(res, query, args...) }) return res.Count, err } diff --git a/model.go b/model.go index 90c1f01bc..2956b1695 100644 --- a/model.go +++ b/model.go @@ -23,8 +23,9 @@ type modelIterable func(*Model) error // that is passed in to many functions. type Model struct { Value - tableName string - As string + tableName string + As string + ignoreTableName bool } // ID returns the ID of the Model. All models must have an `ID` field this is diff --git a/sql_builder.go b/sql_builder.go index 5e3c4cb5a..d99c60227 100644 --- a/sql_builder.go +++ b/sql_builder.go @@ -216,10 +216,12 @@ var columnCacheMutex = sync.RWMutex{} func (sq *sqlBuilder) buildColumns() columns.Columns { tableName := sq.Model.TableName() + asName := sq.Model.As - if asName == "" { + if asName == "" && !sq.Model.ignoreTableName { asName = strings.Replace(tableName, ".", "_", -1) } + acl := len(sq.AddColumns) if acl == 0 { columnCacheMutex.RLock() From bef65df73476dc20a76c235124f584a54ac69133 Mon Sep 17 00:00:00 2001 From: Dylan Boltz Date: Mon, 10 Jun 2019 15:16:20 +0900 Subject: [PATCH 2/6] Added required count(*) aliased name --- finders.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/finders.go b/finders.go index 2b1e6f5da..c6faa5865 100644 --- a/finders.go +++ b/finders.go @@ -332,7 +332,7 @@ func (q Query) CountByField(model interface{}, field string) (int, error) { tmpQuery.Paginator = nil tmpQuery.orderClauses = clauses{} tmpQuery.limitResults = 0 - query, args := tmpQuery.ToSQL(&Model{Value: model, ignoreTableName: true}, "COUNT(*)") + query, args := tmpQuery.ToSQL(&Model{Value: model, ignoreTableName: true}, "COUNT(*) as row_count") //when query contains custom selected fields / executed using RawQuery, // sql may already contains limit and offset From 2021f1f22296c84ff5e59f5f28f73ddf7f6e24e5 Mon Sep 17 00:00:00 2001 From: Dylan Boltz Date: Tue, 11 Jun 2019 07:47:32 +0900 Subject: [PATCH 3/6] Changed count logic to require an optimize flag to be set explicitly before default to that behavior --- finders.go | 29 ++++++++++++++++++++++++----- query.go | 2 ++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/finders.go b/finders.go index c6faa5865..3072ee9e8 100644 --- a/finders.go +++ b/finders.go @@ -332,10 +332,24 @@ func (q Query) CountByField(model interface{}, field string) (int, error) { tmpQuery.Paginator = nil tmpQuery.orderClauses = clauses{} tmpQuery.limitResults = 0 - query, args := tmpQuery.ToSQL(&Model{Value: model, ignoreTableName: true}, "COUNT(*) as row_count") + + var query, countQuery string + var args []interface{} + + if tmpQuery.OptimizeCount && tmpQuery.RawSQL == nil { + tmpQuery.addColumns = []string{} + query, args = tmpQuery.ToSQL(&Model{Value: model, ignoreTableName: true}, + fmt.Sprintf("COUNT(%s) as row_count", field)) + } else { + if tmpQuery.OptimizeCount && tmpQuery.RawSQL != nil { + log(logging.Warn, "Query contains raw SQL; COUNT cannot be optimized") + } + + query, args = tmpQuery.ToSQL(&Model{Value: model}) + } + //when query contains custom selected fields / executed using RawQuery, // sql may already contains limit and offset - if rLimitOffset.MatchString(query) { foundLimit := rLimitOffset.FindString(query) query = query[0 : len(query)-len(foundLimit)] @@ -344,9 +358,14 @@ func (q Query) CountByField(model interface{}, field string) (int, error) { query = query[0 : len(query)-len(foundLimit)] } - // countQuery := fmt.Sprintf("SELECT COUNT(%s) AS row_count FROM (%s) a", field, query) - log(logging.SQL, query, args...) - return q.Connection.Store.Get(res, query, args...) + if tmpQuery.OptimizeCount { + countQuery = query + } else { + countQuery = fmt.Sprintf("SELECT COUNT(%s) AS row_count FROM (%s) a", field, query) + } + + log(logging.SQL, countQuery, args...) + return q.Connection.Store.Get(res, countQuery, args...) }) return res.Count, err } diff --git a/query.go b/query.go index efa276937..7a9b1ca4e 100644 --- a/query.go +++ b/query.go @@ -24,6 +24,7 @@ type Query struct { havingClauses havingClauses Paginator *Paginator Connection *Connection + OptimizeCount bool } // Clone will fill targetQ query with the connection used in q, if @@ -41,6 +42,7 @@ func (q *Query) Clone(targetQ *Query) { targetQ.groupClauses = q.groupClauses targetQ.havingClauses = q.havingClauses targetQ.addColumns = q.addColumns + targetQ.OptimizeCount = q.OptimizeCount if q.Paginator != nil { paginator := *q.Paginator From 1b72747fb7d9460a5eb4b6a73d971a1d16252002 Mon Sep 17 00:00:00 2001 From: Dylan Boltz Date: Tue, 11 Jun 2019 08:04:04 +0900 Subject: [PATCH 4/6] Fixed how count was determining if query contained raw fragments --- finders.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/finders.go b/finders.go index 3072ee9e8..047458da1 100644 --- a/finders.go +++ b/finders.go @@ -335,13 +335,18 @@ func (q Query) CountByField(model interface{}, field string) (int, error) { var query, countQuery string var args []interface{} + var isRaw bool - if tmpQuery.OptimizeCount && tmpQuery.RawSQL == nil { + if tmpQuery.RawSQL != nil && tmpQuery.RawSQL.Fragment != "" { + isRaw = true + } + + if tmpQuery.OptimizeCount && !isRaw { tmpQuery.addColumns = []string{} query, args = tmpQuery.ToSQL(&Model{Value: model, ignoreTableName: true}, fmt.Sprintf("COUNT(%s) as row_count", field)) } else { - if tmpQuery.OptimizeCount && tmpQuery.RawSQL != nil { + if tmpQuery.OptimizeCount && isRaw { log(logging.Warn, "Query contains raw SQL; COUNT cannot be optimized") } From 91e22e562e54d4e682d62f7293840a3c4c900b3a Mon Sep 17 00:00:00 2001 From: Dylan Boltz Date: Tue, 11 Jun 2019 08:41:22 +0900 Subject: [PATCH 5/6] Added tests for executing COUNT with the OptimizeCount flag set --- connection.go | 1 + finders.go | 5 ++++- finders_test.go | 34 ++++++++++++++++++++++++++++++++++ query.go | 1 + sql_builder.go | 1 + 5 files changed, 41 insertions(+), 1 deletion(-) diff --git a/connection.go b/connection.go index fdf91d574..6181af35b 100644 --- a/connection.go +++ b/connection.go @@ -22,6 +22,7 @@ type Connection struct { TX *Tx eager bool eagerFields []string + OptimizeCount bool } func (c *Connection) String() string { diff --git a/finders.go b/finders.go index 047458da1..e38c78405 100644 --- a/finders.go +++ b/finders.go @@ -341,8 +341,11 @@ func (q Query) CountByField(model interface{}, field string) (int, error) { isRaw = true } + // Count can't be optimized if the query contains raw SQL due to ToSQL internals if tmpQuery.OptimizeCount && !isRaw { - tmpQuery.addColumns = []string{} + tmpQuery.addColumns = []string{} // Optimizing Count means giving up selecting any distinct columns. + // This can be changed in the future but will also have to address the issue of + // table aliasing in column names -- AKA reevaluating how Model.ignoreTableName works. query, args = tmpQuery.ToSQL(&Model{Value: model, ignoreTableName: true}, fmt.Sprintf("COUNT(%s) as row_count", field)) } else { diff --git a/finders_test.go b/finders_test.go index 73f1530f2..d016e926d 100644 --- a/finders_test.go +++ b/finders_test.go @@ -669,6 +669,40 @@ func Test_Count(t *testing.T) { }) } +func Test_Count_Optimized(t *testing.T) { + if PDB == nil { + t.Skip("skipping integration tests") + } + transaction(func(tx *Connection) { + r := require.New(t) + + tx.OptimizeCount = true + + user := User{Name: nulls.NewString("Dylan")} + err := tx.Create(&user) + r.NoError(err) + c, err := tx.Count(&user) + r.NoError(err) + r.Equal(c, 1) + + c, err = tx.Where("1=1").CountByField(&user, "distinct id") + r.NoError(err) + r.Equal(c, 1) + // should ignore order in count + + c, err = tx.Order("id desc").Count(&user) + r.NoError(err) + r.Equal(c, 1) + + var uAQ []UsersAddressQuery + _, err = Q(tx).Select("users_addresses.*").LeftJoin("users", "users.id=users_addresses.user_id").Count(&uAQ) + r.NoError(err) + + _, err = Q(tx).Select("users_addresses.*", "users.name", "users.email").LeftJoin("users", "users.id=users_addresses.user_id").Count(&uAQ) + r.NoError(err) + }) +} + func Test_Count_Disregards_Pagination(t *testing.T) { if PDB == nil { t.Skip("skipping integration tests") diff --git a/query.go b/query.go index 7a9b1ca4e..c961adcc8 100644 --- a/query.go +++ b/query.go @@ -184,6 +184,7 @@ func Q(c *Connection) *Query { Connection: c, eager: c.eager, eagerFields: c.eagerFields, + OptimizeCount: c.OptimizeCount, } } diff --git a/sql_builder.go b/sql_builder.go index d99c60227..87b84581b 100644 --- a/sql_builder.go +++ b/sql_builder.go @@ -218,6 +218,7 @@ func (sq *sqlBuilder) buildColumns() columns.Columns { tableName := sq.Model.TableName() asName := sq.Model.As + // If asName is not explicitly set and ignoreTableName is set, then don't us an AS name (alias) if asName == "" && !sq.Model.ignoreTableName { asName = strings.Replace(tableName, ".", "_", -1) } From e02f97a60c75762fe3b81954770ab4990e4d80e0 Mon Sep 17 00:00:00 2001 From: Dylan Boltz Date: Tue, 11 Jun 2019 08:53:23 +0900 Subject: [PATCH 6/6] Formatting --- connection.go | 16 ++++++++-------- query.go | 8 ++++---- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/connection.go b/connection.go index 6181af35b..3bf94067e 100644 --- a/connection.go +++ b/connection.go @@ -15,14 +15,14 @@ var Connections = map[string]*Connection{} // Connection represents all necessary details to talk with a datastore type Connection struct { - ID string - Store store - Dialect dialect - Elapsed int64 - TX *Tx - eager bool - eagerFields []string - OptimizeCount bool + ID string + Store store + Dialect dialect + Elapsed int64 + TX *Tx + eager bool + eagerFields []string + OptimizeCount bool } func (c *Connection) String() string { diff --git a/query.go b/query.go index c961adcc8..7c6849b79 100644 --- a/query.go +++ b/query.go @@ -180,10 +180,10 @@ func (q *Query) Limit(limit int) *Query { // Q will create a new "empty" query from the current connection. func Q(c *Connection) *Query { return &Query{ - RawSQL: &clause{}, - Connection: c, - eager: c.eager, - eagerFields: c.eagerFields, + RawSQL: &clause{}, + Connection: c, + eager: c.eager, + eagerFields: c.eagerFields, OptimizeCount: c.OptimizeCount, } }