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

chore: hoist node.js bindings on top of rust sdk #2879

Merged
merged 59 commits into from
Apr 16, 2024

Conversation

tychoish
Copy link
Contributor

The commit history is wacky because I started this work before the SDK branch was fully merged but the diff is normal.

@greyscaled if you wanna take this for a spin, that'd be cool too.

tychoish added 30 commits March 20, 2024 17:32
bindings/nodejs/src/connection.rs Show resolved Hide resolved
};

Ok(con.clone())
static DEFAULT_CON: OnceCell<Connection> = OnceCell::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably just return a new instance each time. This seems like it could produce some unexpected behavior.

such as.

const db1 = glaredb.connect() 

const db2 = glaredb.connect() 

I'd expect these to be two separate sessions, but this code looks like they'd use a shared connection.

JS isn't really known for it's performance, so the cost of spinning up a new session is likely not a concern. It's what we are already doing, and seems a bit safer a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd expect these to be two separate sessions, but this code looks like they'd use a shared connection.

This is complicated:

  • before 138347b, there was only one glaredb in memory database (particularly confusing for testing, I think, but also users.)
  • as of chore: integrate new rust sdk into python bindings #2850, the python driver has (will have) the semantics you (and I) expect, as above.
  • also as of that PR the glaredb.sql() and glaredb.execute() functions in python, will access this static/global shared db.
  • I believe, that we could have a similar (global) functionality in node, but I'd be happy to drop it. my grep shows that this is never called.

bindings/nodejs/src/connection.rs Outdated Show resolved Hide resolved
bindings/nodejs/src/execution.rs Outdated Show resolved Hide resolved
bindings/nodejs/src/execution.rs Show resolved Hide resolved
bindings/nodejs/src/connection.rs Outdated Show resolved Hide resolved
@tychoish
Copy link
Contributor Author

So this patch mirrors the Python PR, which has a few quirks in service of making it possible to reference python variables as tables in later SQL queries (which means that the result of the sql() and prql() methods need to be re-executable.)

Most of the quirks of this implementation are in service of mirroring the python implementation, but I think if we don't intend to have this API, then we can have a much simpler implementation here.

@tychoish tychoish merged commit beab1fd into main Apr 16, 2024
26 checks passed
@tychoish tychoish deleted the tycho/topic/sdk/7/nodejs-integration branch April 16, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants