SQLite Custom Functions are now supported #25

danramteke · 2021-06-26T17:52:29Z

This PR adds custom function support to sqlite-nio. Both custom functions and custom aggregates are supported. To find out more about these features, please see the SQLite website about them: https://www.sqlite.org/appfunc.html

There are some queries only possible using custom functions. And further, custom function help soften the need for custom builds of SQLite with various parts of the SQLite amalgamation enabled or disabled: one can simply write a custom function instead.

I have adapted this code from https://github.com/groue/GRDB.swift/blob/master/GRDB/Core/DatabaseFunction.swift by @groue. Because GRDB doesn't support Linux or NIO, I was motivated to adapt his code.

Example:

func testFunctionArgumentString() throws {
	let conn = try SQLiteConnection.open(storage: .memory, threadPool: self.threadPool, on: self.eventLoop).wait()
	defer { try! conn.close().wait() }
	let fn = SQLiteCustomFunction("f", argumentCount: 1) { (values: [SQLiteData]) in
		return values[0].string
	}
	try conn.install(customFunction: fn).wait()
	XCTAssertNil(try conn.query("SELECT f(NULL) as result")
								.map { rows in rows[0].column("result")?.string }.wait())
	XCTAssertEqual("foo", try conn.query("SELECT f('foo') as result")
									.map { rows in rows[0].column("result")?.string }.wait())
}

There are many more examples in the new unit test file.

Lastly, is there a swift-format configuration I can use to format my PR to match the Vapor's code style?

0xTim

Initial scan looks good, I'll do a proper review next week. Note the comment about CI

0xTim · 2021-06-26T21:44:33Z

.github/workflows/test.yml

@@ -10,7 +10,7 @@ defaults:
 jobs:
  dependents:
    runs-on: ubuntu-latest
-    container: swift:5.2-bionic


This needs to stay as 5.2 to ensure we don't break 5.2 compatibility

Fixed!

I updated the maxim-lobanov/setup-xcode to v1.2.1 since macOS build won't start.

https://github.com/vapor/sqlite-nio/runs/2922618375?check_suite_focus=true

@0xTim 5.2 compatibility is checked by the main Linux workflows in each repo (at least, that's how it's supposed to work) - I prefer to run the dependent checks in a "current"-version container to reflect the most common usage, since those checks don't have a version matrix.

groue · 2021-06-28T09:25:55Z

I have adapted this code from https://github.com/groue/GRDB.swift/blob/master/GRDB/Core/DatabaseFunction.swift by @groue.

Hello, and thank you for getting inspired from my work 👍

GRDB uses the the MIT license. It is a permissive license, which allows you to do anything you like with the licensed code. The only requirement is:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

The SQLiteCustomFunction.swift file in this PR contains substantial portions of https://github.com/groue/GRDB.swift/blob/v5.8.0/GRDB/Core/DatabaseFunction.swift: please include the GRDB copyright and permission notices in your derived work.

danramteke · 2022-02-01T16:46:23Z

Any updates? @gwynne @0xTim

0xTim

LGTM other than the log levels. I'll let @gwynne have a look before merging though

Sources/SQLiteNIO/SQLiteConnection.swift

danramteke · 2022-02-03T04:33:36Z

Wonderful. I committed both of your suggested changes, @0xTim.

0xTim

Sorry it's taken so long to get to this! The only thing I think is missing is a NOTICES.txt for attribution to GRDB. IANAL but this is what we've done in other projects, e.g. https://github.com/vapor/jwt-kit/blob/main/NOTICES.txt

Once added I'll merge

Austinpayne · 2022-10-24T22:54:16Z

@danramteke Can I help get this across the finish line? I am interested in this functionality as well. I'd be happy to fill out the NOTICES.txt if you provided me access to your branch. Alternatively, I can create a merge to your original branch.

danramteke · 2022-10-26T15:11:18Z

@Austinpayne had no idea anyone else wanted this, thanks for the nudge!

danramteke · 2022-10-26T15:12:33Z

@0xTim I did my best to add a NOTICES.txt as requested.

Austinpayne · 2022-10-26T23:10:08Z

@Austinpayne had no idea anyone else wanted this, thanks for the nudge!

Thank you for implementing it! This unlocks some very interesting use cases and I actually had started on an implementation before seeing yours. Your PR is much more feature rich so really looking forward to seeing in mainlined. Thank you @groue as well for the original implementation!

0xTim

Thanks!

0xTim · 2022-10-27T15:11:24Z

@gwynne anything I've missed?

danramteke · 2022-11-28T18:20:50Z

@0xTim @gwynne following up after 1 month 👀

codecov-commenter · 2022-12-04T05:40:19Z

Codecov Report

Merging #25 (332a713) into main (6552d66) will increase coverage by 19.75%.
The diff coverage is 66.45%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #25       +/-   ##
===========================================
+ Coverage   48.03%   67.78%   +19.75%     
===========================================
  Files           7        8        +1     
  Lines         535      832      +297     
===========================================
+ Hits          257      564      +307     
+ Misses        278      268       -10

Impacted Files	Coverage Δ
Sources/SQLiteNIO/SQLiteError.swift	`27.39% <0.00%> (+27.39%)`	⬆️
Sources/SQLiteNIO/SQLiteStatement.swift	`92.24% <ø> (+12.40%)`	⬆️
Sources/SQLiteNIO/SQLiteConnection.swift	`64.61% <66.66%> (+3.27%)`	⬆️
Sources/SQLiteNIO/SQLiteCustomFunction.swift	`84.06% <84.06%> (ø)`
Sources/SQLiteNIO/SQLiteData.swift	`83.33% <97.36%> (+55.20%)`	⬆️

... and 1 file with indirect coverage changes

gwynne

I've commented inline about several minor issues. I'm also very uncomfortable with the amount of manual memory management being thrown around; I'll try to persuade @Lukasa to take a look at what's going on in that regard.

That concern aside, the overall code seems pretty solid and there's no question that having this functionality is both useful and desirable; if we can address the above, this will be good to go!

Sources/SQLiteNIO/SQLiteConnection.swift

Sources/SQLiteNIO/SQLiteDataConvertible.swift

Sources/SQLiteNIO/SQLiteError.swift

Tests/SQLiteNIOTests/SQLiteCustomFunctionTests.swift

Sources/SQLiteNIO/SQLiteCustomFunction.swift

Lukasa · 2022-12-05T08:51:13Z

Sources/SQLiteNIO/SQLiteCustomFunction.swift

+			nil, nil, nil, nil, nil)
+
+		guard code == SQLITE_OK else {
+			// Assume a bug: there is no point throwing any error.


This is a weird comment above this code.

Lukasa · 2022-12-05T08:51:20Z

Sources/SQLiteNIO/SQLiteCustomFunction.swift

+			})
+
+		guard code == SQLITE_OK else {
+			// Assume a bug: there is no point throwing any error.


This is a weird comment above this bug.

Lukasa · 2022-12-05T08:55:06Z

Sources/SQLiteNIO/SQLiteCustomFunction.swift

+				assert(!aggregateContext.hasErrored) // assert SQLite behavior
+				do {
+					let arguments = (0..<Int(argc)).map { index in
+						SQLiteData(sqliteValue: argv.unsafelyUnwrapped[index]!)


There's no justification for unsafelyUnwrapped here.

Lukasa · 2022-12-05T09:34:37Z

Sources/SQLiteNIO/SQLiteCustomFunction.swift

+				aggregateContextBufferP.copyMemory(from: $0)
+			}
+			return aggregateContextU
+		}


I'm not wild about this function. I think there are a few places where it's being excessively clever.

First, I really don't love that it's a bit vague about what it's storing. In the initialization branch it stores the bytes of an OpaquePointer into the buffer, but in the other branch it just immediately type-puns to Unmanaged<AggregateContext>. That works, but it's not great: we should be consistent in the branches. I'd probably prefer we use OpaquePointer and do the full Unmanaged dance in both branches, rather than rely on Unmanaged being bitwise-takeable (though I think it probably is).

The thing I like much less is the detection of non-zero bytes in the buffer as a mechanism for working out if the memory is initialized. Fortunately for us, we don't need to do that. If we define the sqlite_aggregate_context as containing OpaquePointer? (or, alternatively, Unmanaged<AggregateContext>?, though I like that less) then we can use != nil to detect the uninitialised case. This is way safer and less prone to error, as well as being in line with how the API expects to be used in C code.

Finally, this function returns an Unmanaged<AggregateContext>, which sorta begs to be leaked. I'd rather that this function took a "consuming" parameter that decided whether it consumed the unbalanced retain or not.

I'm looking at this review with the greatest interest 😋 This code was written 5+ years ago and never had the privilege of multiple pairs of eyes. If you don't mind, I'd be happy to learn from your experience here.

I understand what the reviewer is saying. I'm open to help for re-implementing this function 💜

I've taken a crack at it, hopefully Cory won't tell me I've made things even worse 😅

Sources/SQLiteNIO/SQLiteData.swift

danramteke · 2022-12-05T23:42:00Z

I have addressed most of the code comments from @gwynne & @Lukasa. Could you please allow the GitHub Action to run?

Austinpayne · 2023-02-13T21:58:41Z

@danramteke I'm confused, you appear to have reverted the changes of #35 in this PR. Was that intentional?

gwynne · 2023-02-13T22:27:25Z

That's due to this branch not having been updated in a while. The conflicts need to be resolved; the seeming "revert" is just an artifact of that. @danramteke, can you get this up to date with the changes from main?

gwynne · 2023-02-13T22:28:33Z

And also @Lukasa could you take a peek and let us know if your issues were resolved? 🙂

danramteke · 2023-02-16T15:49:45Z

@Austinpayne yep, @gwynne is right, it's the result of merge conflicts

danramteke · 2023-02-22T16:34:18Z

If anyone can help get this PR over the finish line, please lend a helping hand, as my bandwidth is limited at the moment.

Remaining tasks:

This comment SQLite Custom Functions are now supported #25 (comment)
updating with changes on the main branch

# Conflicts: # .github/workflows/test.yml # Package.swift # Sources/CSQLite/include/sqlite3.h # Sources/CSQLite/sqlite3.c # Sources/CSQLite/version.txt # Sources/SQLiteNIO/SQLiteConnection.swift # Sources/SQLiteNIO/SQLiteData.swift # Tests/SQLiteNIOTests/SQLiteNIOTests.swift

gwynne · 2023-02-22T19:19:36Z

@danramteke @Lukasa Okay - I rebased onto main (as it is in your fork) to get rid of the mixed merge commits (merging from upstream main was getting messed up by the merge revert on the branch), fixed up the whitespace confusion, merged upstream main in so everything's completely up to date, and most importantly, I reimplemented the umanagedAggregateContext() method to - I hope! - address Cory's very valid concerns.

Cory, did I get it even remotely right? 🙏

gwynne · 2023-02-22T19:21:34Z

P.S.: Yes, Cory, I know I didn't make the method optionally consuming like you wanted, but the behavior of SQLite with regards to aggregate contexts during xFinal is so weird I wasn't entirely good with the idea of tinkering more than necessary 😕

gwynne · 2023-02-22T20:00:06Z

Ugh, GitHub’s UI some days… 😬

Lukasa · 2023-02-22T20:01:59Z

What happened to the indentation in SQLiteCustomFunction.swift? It's all over the place, the code is extraordinarily hard to read.

gwynne · 2023-02-22T20:11:39Z

What happened to the indentation in SQLiteCustomFunction.swift? It's all over the place, the code is extraordinarily hard to read.

I thought I managed to fix that 😕 Heck with it, I’ll give it a swiftformat pass once I fix the 5.6 build

Sources/SQLiteNIO/SQLiteCustomFunction.swift

Sources/SQLiteNIO/SQLiteData.swift

gwynne · 2023-02-24T18:24:54Z

@groue It seems as if I managed to stumble my way into what Cory was looking for in terms of improvements here - please feel free to use it in GRDB!

groue · 2023-02-24T20:31:00Z

@groue It seems as if I managed to stumble my way into what Cory was looking for in terms of improvements here - please feel free to use it in GRDB!

That's lovely 😊 I certainly follow this issue and your improvements. The SQLite api regarding custom functions and aggregates is awful (an exception because SQLite is usually a very nice C library). Thank you very much again.

danramteke · 2023-02-26T15:14:57Z

Thank you @gwynne !!!

gwynne · 2023-03-13T05:39:22Z

Deliberately merging without a semver- label, will release manually.

0xTim reviewed Jun 26, 2021

View reviewed changes

0xTim requested changes Feb 2, 2022

View reviewed changes

Sources/SQLiteNIO/SQLiteConnection.swift Outdated Show resolved Hide resolved

Sources/SQLiteNIO/SQLiteConnection.swift Outdated Show resolved Hide resolved

0xTim requested changes Jun 29, 2022

View reviewed changes

0xTim added the semver-minor Contains new APIs label Jun 29, 2022

0xTim approved these changes Oct 27, 2022

View reviewed changes

gwynne requested changes Dec 4, 2022

View reviewed changes

Lukasa reviewed Dec 5, 2022

View reviewed changes

gwynne requested a review from Lukasa December 6, 2022 00:18

Daniel R added 6 commits February 22, 2023 11:16

wip - integrating from GRDB

6d92047

database function passes

f6d491e

error status code

0efcbb6

aggregate succeeded

333d9e0

reneames

f82e5ba

ad/remove with eventloop

fe5bf06

Apply Cory's feedback to the unmanagedAggregateContext() method.

02648dc

gwynne force-pushed the custom-functions branch from f667516 to 02648dc Compare February 22, 2023 19:13

Attempt to fix build for Swift <5.7

f9c4a3f

gwynne requested review from gwynne and Lukasa and removed request for Lukasa February 22, 2023 19:58

gwynne requested review from Lukasa and removed request for Lukasa February 22, 2023 19:59

gwynne added 2 commits February 22, 2023 15:44

Normalize whitespace. \t oughtta go the way of EBCDIC...

1f7fefb

Finish fixing Swift 5.5/5.6 build

8fd9fb9

Lukasa reviewed Feb 24, 2023

View reviewed changes

Sources/SQLiteNIO/SQLiteCustomFunction.swift Outdated Show resolved Hide resolved

Sources/SQLiteNIO/SQLiteData.swift Outdated Show resolved Hide resolved

Address PR feedback

9e20735

gwynne approved these changes Feb 24, 2023

View reviewed changes

Merge branch 'main' into custom-functions

332a713

gwynne removed the semver-minor Contains new APIs label Mar 13, 2023

gwynne merged commit 596b680 into vapor:main Mar 13, 2023

SQLite Custom Functions are now supported #25

SQLite Custom Functions are now supported #25

Conversation

danramteke commented Jun 26, 2021 • edited by 0xTim Loading

0xTim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danramteke Jun 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groue commented Jun 28, 2021 • edited Loading

danramteke commented Feb 1, 2022

0xTim left a comment

Choose a reason for hiding this comment

danramteke commented Feb 3, 2022

0xTim left a comment

Choose a reason for hiding this comment

Austinpayne commented Oct 24, 2022 • edited Loading

danramteke commented Oct 26, 2022

danramteke commented Oct 26, 2022

Austinpayne commented Oct 26, 2022 • edited Loading

0xTim left a comment

Choose a reason for hiding this comment

0xTim commented Oct 27, 2022

danramteke commented Nov 28, 2022

codecov-commenter commented Dec 4, 2022 • edited Loading

Codecov Report

gwynne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groue Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danramteke commented Dec 5, 2022

Austinpayne commented Feb 13, 2023

gwynne commented Feb 13, 2023

gwynne commented Feb 13, 2023

danramteke commented Feb 16, 2023

danramteke commented Feb 22, 2023 • edited by gwynne Loading

gwynne commented Feb 22, 2023

gwynne commented Feb 22, 2023

gwynne commented Feb 22, 2023

Lukasa commented Feb 22, 2023

gwynne commented Feb 22, 2023

gwynne commented Feb 24, 2023

groue commented Feb 24, 2023 • edited Loading

danramteke commented Feb 26, 2023

gwynne commented Mar 13, 2023

danramteke commented Jun 26, 2021 •

edited by 0xTim

Loading

danramteke Jun 26, 2021 •

edited

Loading

groue commented Jun 28, 2021 •

edited

Loading

Austinpayne commented Oct 24, 2022 •

edited

Loading

Austinpayne commented Oct 26, 2022 •

edited

Loading

codecov-commenter commented Dec 4, 2022 •

edited

Loading

groue Dec 5, 2022 •

edited

Loading

danramteke commented Feb 22, 2023 •

edited by gwynne

Loading

groue commented Feb 24, 2023 •

edited

Loading