Implement https://sqlite.org/c3ref/vtab_in.html #126
Conversation
implementation to Value. This enables vtables to receive all IN values at once in the Filter function.
| // All iterates all values of an IN operator | ||
| // https://www.sqlite.org/c3ref/vtab_in.html | ||
| // https://www.sqlite.org/c3ref/vtab_in_first.html | ||
| func (v Value) All() iter.Seq[Value] { |
There was a problem hiding this comment.
While I appreciate the simplicity of the iterator interface, I'm not quite sold on this API for a few reasons:
- It obscures errors.
- It's not a common enough operation to justify being a method IMO.
- The lifetime of the
Valueobjects from these functions is somewhat tricky and that complexity is not being surfaced to the user. - I'm not fond of giving the user ability to call this function outside of the context of best index, since SQLite docs say the behavior is undefined, which is worse than giving an error.
I don't have a great counter-proposal for this design, but I didn't want to block the feedback on me coming up with something.
There was a problem hiding this comment.
Yeah good points. What if this were a method on the VTCursor interface instead? Then it would be much harder to use outside the Filter function (the only place sqlite_vtab_in_{first,next} are valid to use).
Also do you think the golang iter interface is ok or passing in a callback function is preferred?
| @@ -38,6 +38,19 @@ func ExampleVTable() { | |||
| log.Fatal(err) | |||
| } | |||
|
|
|||
| err = sqlitex.ExecuteTransient( | |||
There was a problem hiding this comment.
This doesn't feel like something that needs to be included in an introductory example. IIUC SQLite will fall back to a simpler set of indices and still operate without this.
It definitely still needs testing, but shouldn't be in this example.
| // InOpAllAtOnce is true if the constraint is an IN operator | ||
| // that can be processed all at once | ||
| InOpAllAtOnce bool |
There was a problem hiding this comment.
nit: We abbreviate operator for common usages, but this is less common, so let's spell it out here.
| // InOpAllAtOnce is true if the constraint is an IN operator | |
| // that can be processed all at once | |
| InOpAllAtOnce bool | |
| // InOperatorAllAtOnce is true if the constraint is an IN operator | |
| // that can be processed all at once | |
| InOperatorAllAtOnce bool |
| // Set InOpAllAtOnce to true if the constraint is an IN operator and the | ||
| // Filter method should receive all values at once. | ||
| InOpAllAtOnce bool |
There was a problem hiding this comment.
nit: We abbreviate operator for common usages, but this is less common, so let's spell it out here.
| // Set InOpAllAtOnce to true if the constraint is an IN operator and the | |
| // Filter method should receive all values at once. | |
| InOpAllAtOnce bool | |
| // If InOperatorAllAtOnce is true, then the Filter method | |
| // will receive all values of an IN operator at once. | |
| InOperatorAllAtOnce bool |
The doc comment should also include instructions on how to access the values.
| ptr := (*lib.Sqlite3_index_constraint_usage)(unsafe.Pointer(aConstraintUsage)) | ||
| ptr.FargvIndex = int32(u.ArgvIndex) | ||
| if u.Omit { | ||
| ptr.Fomit = 1 | ||
| } else { | ||
| ptr.Fomit = 0 | ||
| } | ||
| if u.InOpAllAtOnce { | ||
| lib.Xsqlite3_vtab_in(tls, infoPtr, int32(i), 1) |
There was a problem hiding this comment.
Perhaps check the return value of this? IIUC if it's zero, that indicates that the library user has improperly used this call.
Sqlite 3.38 added the ability for vtables to opt into receiving all values of an IN operator all at once in filter, instead of one at a time. This can enable more efficient fetching of rows with batching or prefetching, etc. Here is my first attempt at adding this to go-sqlite, the Value struct now can be iterated using the go 1.23 range iter. Includes a unit test update.
c.f. sqlite vtab_in APIs:
https://sqlite.org/c3ref/vtab_in.html
https://sqlite.org/c3ref/vtab_in_first.html