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

fix(datastore): eager loaded associations depth #520

Merged
merged 10 commits into from
Jun 5, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ extension ModelField: SQLColumn {
if let namespace = namespace {
column = "\(namespace).\(column)"
}
return "as \(column.quoted())"
return column.quoted()
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,78 +9,49 @@ import Amplify
import Foundation
import SQLite

/// Represents a `select` SQL statement associated with a `Model` instance and
/// optionally composed by a `ConditionStatement`.
struct SelectStatement: SQLStatement {

let modelType: Model.Type
let conditionStatement: ConditionStatement?
let paginationInput: QueryPaginationInput?

// TODO remove this once sorting support is added to DataStore
// Used by plugin to order and limit results for system table queries
let additionalStatements: String?
let namespace = "root"

init(from modelType: Model.Type,
predicate: QueryPredicate? = nil,
paginationInput: QueryPaginationInput? = nil,
additionalStatements: String? = nil) {
self.modelType = modelType

var conditionStatement: ConditionStatement?
if let predicate = predicate {
let statement = ConditionStatement(modelType: modelType,
predicate: predicate,
namespace: namespace[...])
conditionStatement = statement
}
self.conditionStatement = conditionStatement
self.paginationInput = paginationInput
self.additionalStatements = additionalStatements
}

var stringValue: String {
/// Support data structure used to hold information about `SelectStatement` than
/// can be used later to parse the results.
struct SelectStatementMetadata {

typealias ColumnMapping = [String: (ModelSchema, ModelField)]

let statement: String
let columnMapping: ColumnMapping
let bindings: [Binding?]

// TODO remove additionalStatements once sorting support is added to DataStore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe create a github issue for this TODO to track?

static func get(from modelType: Model.Type,
Copy link
Contributor

@royjit royjit Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename to something different than 'get'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure thing, suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this just be the constructor for SelectStatementMetadata?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make metaData(from:), statementMetaData(from:) or why not an init method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this just be the constructor for SelectStatementMetadata?

using the constructor is tricky because I have to execute the entire logic inside the constructor (no other functions, etc) because I need to initialize any struct property.

That's why I went with the static factory function, otherwise the constructor becomes a mess.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also thinking .selectStatementMetadata(from:)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's an internal API, so I renamed to .metadata(), feel free to change that later as it has no impact on customers.

predicate: QueryPredicate? = nil,
paginationInput: QueryPaginationInput? = nil,
additionalStatements: String? = nil) -> SelectStatementMetadata {
let rootNamespace = "root"
let schema = modelType.schema
let fields = schema.columns
let tableName = schema.name
var columnMapping: ColumnMapping = [:]
var columns = fields.map { field -> String in
return field.columnName(forNamespace: namespace) + " " + field.columnAlias()
columnMapping.updateValue((schema, field), forKey: field.name)
return field.columnName(forNamespace: rootNamespace) + " as " + field.columnAlias()
}

// eager load many-to-one/one-to-one relationships
var joinStatements: [String] = []
for foreignKey in schema.foreignKeys {
let associatedModelType = foreignKey.requiredAssociatedModel
let associatedSchema = associatedModelType.schema
let associatedTableName = associatedModelType.schema.name

// columns
let alias = foreignKey.name
let associatedColumn = associatedSchema.primaryKey.columnName(forNamespace: alias)
let foreignKeyName = foreignKey.columnName(forNamespace: "root")

// append columns from relationships
columns += associatedSchema.columns.map { field -> String in
return field.columnName(forNamespace: alias) + " " + field.columnAlias(forNamespace: alias)
}

let joinType = foreignKey.isRequired ? "inner" : "left outer"

joinStatements.append("""
\(joinType) join \(associatedTableName) as \(alias)
on \(associatedColumn) = \(foreignKeyName)
""")
}
let (joinedColumns, joinStatements, joinedColumnMapping) = joins(from: schema)
columns += joinedColumns
columnMapping.merge(joinedColumnMapping) { _, new in new }

var sql = """
select
\(joinedAsSelectedColumns(columns))
from \(tableName) as root
from \(tableName) as "\(rootNamespace)"
\(joinStatements.joined(separator: "\n"))
""".trimmingCharacters(in: .whitespacesAndNewlines)

if let conditionStatement = conditionStatement {
var bindings: [Binding?] = []
if let predicate = predicate {
let conditionStatement = ConditionStatement(modelType: modelType,
predicate: predicate,
namespace: rootNamespace[...])
bindings.append(contentsOf: conditionStatement.variables)
sql = """
\(sql)
where 1 = 1
Expand All @@ -101,12 +72,81 @@ struct SelectStatement: SQLStatement {
\(paginationInput.sqlStatement)
"""
}
return SelectStatementMetadata(statement: sql,
columnMapping: columnMapping,
bindings: bindings)
}

/// Tuple that represents a pair of `columns` and `statements`
private typealias JoinStatements = ([String], [String], ColumnMapping)

/// Walk through the associations recursively to generate join statements.
///
/// Implementation note: this should be revisited once we define support
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are doing eager loading now right? Let us add that to the comment.

/// for explicit `eager` vs `lazy` associations.
private static func joins(from schema: ModelSchema) -> JoinStatements {
drochetti marked this conversation as resolved.
Show resolved Hide resolved
var columns: [String] = []
var joinStatements: [String] = []
var columnMapping: ColumnMapping = [:]

func visitAssociations(node: ModelSchema, namespace: String = "root") {
for foreignKey in node.foreignKeys {
let associatedModelType = foreignKey.requiredAssociatedModel
let associatedSchema = associatedModelType.schema
let associatedTableName = associatedModelType.schema.name

// columns
let alias = namespace == "root" ? foreignKey.name : "\(namespace).\(foreignKey.name)"
let associatedColumn = associatedSchema.primaryKey.columnName(forNamespace: alias)
let foreignKeyName = foreignKey.columnName(forNamespace: namespace)

// append columns from relationships
columns += associatedSchema.columns.map { field -> String in
columnMapping.updateValue((associatedSchema, field), forKey: "\(alias).\(field.name)")
return field.columnName(forNamespace: alias) + " as " + field.columnAlias(forNamespace: alias)
}

let joinType = foreignKey.isRequired ? "inner" : "left outer"

joinStatements.append("""
\(joinType) join \(associatedTableName) as "\(alias)"
on \(associatedColumn) = \(foreignKeyName)
""")
Comment on lines +114 to +117
Copy link
Contributor

@wooj2 wooj2 Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being ultra paranoid, do we care to filter out names of tables that might impact these statements. Like... it would be bad if someone had a table called Null or as (or something malicious)... maybe something to do for another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are escaped with double quotes, so you can even pass a SQL statement there and nothing would happen.

that said, the names here are a result of the models, so only valid Swift identifiers can be passed here.

visitAssociations(node: associatedModelType.schema,
namespace: alias)
}
}
visitAssociations(node: schema)

return (columns, joinStatements, columnMapping)
}

}

/// Represents a `select` SQL statement associated with a `Model` instance and
/// optionally composed by a `ConditionStatement`.
struct SelectStatement: SQLStatement {

let modelType: Model.Type
let metadata: SelectStatementMetadata

init(from modelType: Model.Type,
predicate: QueryPredicate? = nil,
paginationInput: QueryPaginationInput? = nil,
additionalStatements: String? = nil) {
self.modelType = modelType
self.metadata = .get(from: modelType,
Copy link
Contributor

@royjit royjit Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot happens here in the init method, and I think this code path is sync with the developers code Amplify.DataStore.query. Is this an expensive logic that can affect the query api?

In future we should add some performance test around this.

Copy link
Contributor Author

@drochetti drochetti Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to performance benchmarks

Is this an expensive logic that can affect the query api?

No, it's not expensive. It generates a string based on a Swift struct. Of course there are tons of optimizations we could do, like caching the queries, etc. But as of now I have no concerns.

predicate: predicate,
paginationInput: paginationInput,
additionalStatements: additionalStatements)
}

return sql
var stringValue: String {
metadata.statement
}

var variables: [Binding?] {
return conditionStatement?.variables ?? []
metadata.bindings
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,12 @@ import Amplify
import SQLite

extension Statement {
public func convert(toUntypedModel modelType: Model.Type) throws -> [Model] {
func convert(toUntypedModel modelType: Model.Type,
using statement: SelectStatement) throws -> [Model] {
var models = [Model]()
var convertedCache: ConvertCache = [:]

for row in self {
let modelValues = try mapEach(row: row,
to: modelType,
cache: &convertedCache)
let modelValues = try convert(row: row, to: modelType, using: statement)
let untypedModel = try convert(toAnyModel: modelType, modelDictionary: modelValues)
models.append(untypedModel)
}
Expand Down
Loading