-
Notifications
You must be signed in to change notification settings - Fork 8
feat(sqlite): implement sqlite in wasip2 #25
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
base: wasip2
Are you sure you want to change the base?
Conversation
Worth squashing commits? |
Squished |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a super smooth experience! I left a few comments but I think they're nits except possibly the build command line?
allowed_outbound_hosts = [] | ||
sqlite_databases = ["default"] | ||
[component.sqlite.build] | ||
command = "tinygo build -target=wasip2 --wit-package $(go list -mod=readonly -m -f '{{.Dir}}' github.com/spinframework/spin-go-sdk/v3)/wit --wit-world http-trigger -gc=leaking -o main.wasm main.go" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the $(go list ...)
work on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, go list
should work, but not sure about $(go list...)
. I had opened a ticket ydnar/wasi-http-go#22 to check with other folks if there is a better way to do this.
case "text": | ||
ret[i] = *v.Text() | ||
case "blob": | ||
// TODO: check this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this now TODONE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, It's not. For some reason blob
columns are being returned as text
. I'm still tracing it.
v3/sqlite/sqlite.go
Outdated
rows := &rows{ | ||
columns: cols, | ||
rows: allrows, | ||
len: int(rowLen), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to hold this separately - e.g. is len(allrows)
expensive to calculate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was copy pasta that I left in because it spares the cost of len(allrows)
on each iteration of .Next()
. TBH I'm not sure if it saves all that much.
v3/examples/sqlite/main.go
Outdated
db := sqlite.Open("default") | ||
defer db.Close() | ||
|
||
_, err := db.Query("REPLACE INTO pets VALUES (4, 'Maya', ?, false);", "bananas") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we pls also add init schema in this example. I tried running it and ran into no such table: pets
. Also if you think that is out of scope, maybe a small readme file to initiate that db manually would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually now that i tried creating table myself, its not that hard.
sqlite3 .spin/sqlite_db.db
<copy paste from db/pets.sql>
so feel free to ignore my previous comment 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spin Can Do That! spin up --sqlite @db/pets.sql
08fd1cc
to
c188a77
Compare
Signed-off-by: Adam Reese <[email protected]>
c188a77
to
27c9b9d
Compare
No description provided.