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

serialize DuckDBClient.sql #1728

Closed
wants to merge 1 commit into from
Closed

serialize DuckDBClient.sql #1728

wants to merge 1 commit into from

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Oct 8, 2024

Ref #1469. This uses a WeakMap keyed by the string literal strings, effectively creating a queue per SQL cell and per sql tagged template literal. I’d prefer to implement a more general solution that also helps out with e.g. concurrent fetch (maybe in the Observable Runtime?), but this is an easy fix for probably the most common instance of this problem. The performance of the mag slider on the SQL page is dramatically improved under fast interaction.

@mbostock mbostock requested a review from Fil October 8, 2024 00:06
@Fil
Copy link
Contributor

Fil commented Oct 8, 2024

The crucial thing here is that even if another cell uses the same query (SELECT * FROM x WHERE y), its strings will not be === to those of the first query, because a different array will have been created; so no queuing happens between the two independent queries. 👏

Fil
Fil previously approved these changes Oct 8, 2024
@mbostock mbostock marked this pull request as draft October 8, 2024 21:05
@mbostock
Copy link
Member Author

mbostock commented Oct 8, 2024

Unfortunately I think this approach has a fundamental flaw as exhibited in this example code:

Promise.all([1, 2, 3].map((i) => sql`SELECT ${i}`))

This code will throw Error: invalidated because the i = 1 and i = 2 cases are “invalidated” by the subsequent i = 3. In other words, the approach in this PR works only if the sql template literal is invoked only once per code block, and any case in which the sql template literal exists within a loop will cause problems.

We could still adapt this fix for SQL cells specifically (by changing the generated code to sql.queue or something), but perhaps I should just look into making the more general runtime-level fix.

@Fil
Copy link
Contributor

Fil commented Oct 8, 2024

Ah… bummer.

If you write Promise.all([1, 2, 3].map((i) => sql(["SELECT", ""], i))) instead it works, because it's a new array each time. MDN explains the difference: “For any particular tagged template literal expression, the tag function will always be called with the exact same literal array, no matter how many times the literal is evaluated.”

Could we salvage this approach if we somehow passed the variable's v._version to know if we are running in the same call or in a subsequent one?

@Fil Fil dismissed their stale review November 13, 2024 08:32

obsolete

@Fil
Copy link
Contributor

Fil commented Nov 13, 2024

I believe this was superseded by #1748

@Fil Fil closed this Nov 13, 2024
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