Skip to content

Adds multirow insert method #153

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

Merged
merged 2 commits into from
Sep 23, 2024

Conversation

NeedleInAJayStack
Copy link
Contributor

@NeedleInAJayStack NeedleInAJayStack commented Jul 29, 2022

These changes are now available in 3.32.0

This adds functionality to do a multi-row inserts with a single values method call.

Previously, to insert multiple rows a user had to call values repeatedly:

db.insert(into: "planets")
    .columns(["name", "color"])
    .values([SQLBind("Jupiter"), SQLBind("orange")])
    .values([SQLBind("Mars"), SQLBind("red")])
    .run()

This was a bit awkward when inserting rows from an array, where an instance of the builder had to be saved off and edited:

let rows: [[SQLExpression]]  = [[...], [...], ...]
let builder = db.insert(into: "planets")
    .columns(["name", "color"])
for row in rows {
    builder.values(row)
}
builder.run()

This MR simplifies the mutli-row insert situation by adding a values method overload that accepts a nested array:

db.insert(into: "planets")
    .columns(["name", "color"])
    .values([[SQLBind("Jupiter"), SQLBind("orange")], [SQLBind("Mars"), SQLBind("red")]])
    .run()

let rows  = [[...], [...], ...]
db.insert(into: "planets")
    .columns(["name", "color"])
    .values(rows)
    .run()

NOTE

This functionality was only added to the SQLExpression version of values, NOT the Encodable version of values. There are known issues with [Encodable] conforming to Encodable that prevent adequate type checks.

For example, if a values(_ rows: [[Encodable]]) is defined, it is undeterministic since the same call would also match values(_ rows: [Encodable]). If instead we change values(_ rows: [Encodable]) to detect [[Encodable]] cases at runtime, then it is difficult to guarantee that a caller hasn't mixed Encodable and [Encodable] values.

@NeedleInAJayStack NeedleInAJayStack changed the title feature: Adds multirow insert method DRAFT: Adds multirow insert method Jul 29, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (3c8c6aa) to head (e2b955b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #153   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          100       100           
  Lines         2635      2636    +1     
=========================================
+ Hits          2635      2636    +1     
Files with missing lines Coverage Δ
...it/Builders/Implementations/SQLInsertBuilder.swift 100.00% <100.00%> (ø)

@NeedleInAJayStack NeedleInAJayStack changed the title DRAFT: Adds multirow insert method Adds multirow insert method Jul 30, 2022
@0xTim 0xTim requested a review from gwynne August 1, 2022 09:45
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

I've been meaning to upstream basically this exact helper myself! 🙂 I have two nits:

  1. I'd rather it not be just another overload of values(_:), but have a unique name - consider an attempt to insert into a table with a single column of array type. I named my version multiValues(_:), but I'm not at all convinced that's an ideal name either and would actually welcome a bit of bikeshedding on the matter.
  2. My implementation is a bit more generic:
    extension SQLInsertBuilder {
        @discardableResult
        public func multiValues<S1, S2>(_ valueSets: S1) -> Self
            where S1: Sequence, S2: Sequence,
                  S1.Element == S2, S2.Element == SQLExpression
        {
            valueSets.reduce(self) { $0.values(Array($1)) }
        }
    }
    Even though the input gets converted back to Array in the end regardless, accepting generic sequences allows for cleaner syntax when using, for example, the methods from swift-algorithms to process the data to be inserted.

@NeedleInAJayStack
Copy link
Contributor Author

welcome a bit of bikeshedding on the matter

I think multiValues(_:) is a fine name. Along the same vein, allValues seems like a reasonable option. However, I think I prefer rows(_:) or rowValues(_:), since I feel like they imply that all sequences should have equal length and correspond to the column ordering.

@NeedleInAJayStack
Copy link
Contributor Author

@gwynne Sorry it took so long to get back to this one. I've adjusted it to use your implementation (which is great!), and named it rows. Again, I'm happy to switch to multiValues if you prefer that. Thanks!

@gwynne
Copy link
Member

gwynne commented Sep 22, 2024

@NeedleInAJayStack Any chance you'd be able to update this to resolve the conflicts? I'd be willing to merge it as SQLKit 3's last new feature.

@NeedleInAJayStack
Copy link
Contributor Author

Sure, I can have that done in the next day or so if that works

@NeedleInAJayStack
Copy link
Contributor Author

Hey @gwynne, I've resolved the merge conflicts and updated the tests to the new test formatting. I wasn't sure if any of the tests in the original PR were obsoleted by new work, so just let me know if you'd like any removed/organized differently. Thanks!

Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

Nope, everything looks fine, thanks for your work on this!

@gwynne gwynne added enhancement New feature or request semver-minor Contains new APIs labels Sep 23, 2024
@gwynne gwynne merged commit e0b35ff into vapor:main Sep 23, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-minor Contains new APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants