feat(structured data): query DB v0 #2499

fontanierh · 2023-11-13T11:21:32Z

#2211

Not particularly optimized, no caching etc..

spolu

Made a first pass, will need to review more once addressed 👍

core/src/databases/database.rs

core/src/databases/table_schema.rs

spolu

LGTM

Not a big fan of the ok_or_else I generally go with match cases for that, it's a bit more pedantic but I find slightly easier to read. But no action required. This is valid Rust 👍

fontanierh · 2023-11-13T17:01:58Z

Not a big fan of the ok_or_else I generally go with match cases for that, it's a bit more pedantic but I find slightly easier to read

Ah I was starting to enjoy them !
I can remove

spolu · 2023-11-13T17:10:07Z

Really feel free to keep them! 👍

statements

core/src/databases/database.rs

spolu

Looks overall good. Happy to make a pass on it as well once ready. I think we can optimize a bunch of things a bit more to align with the experiment we did?

core/src/databases/database.rs

core/Cargo.toml

core/src/databases/database.rs

spolu

Starting to look good. I left some comments but overall this looks great.

I'm quite bearish on the untyped jump through Value on your wayt to sql Would much rather have the kind of types we had in the experiment + you can probably handle the Boolean <-> 1/0 in the ToSql FromSql implementation of that which would be much much cleaner?

core/Cargo.toml

core/src/databases/database.rs

core/Cargo.toml

core/src/databases/table_schema.rs

core/src/databases/database.rs

fontanierh · 2023-11-16T09:19:46Z

I'm quite bearish on the untyped jump through Value on your wayt to sql Would much rather have the kind of types we had in the experiment + you can probably handle the Boolean <-> 1/0 in the ToSql FromSql implementation of that which would be much much cleaner?

What we had in the experiment is equally typed as a Value AFAICT ? Not sure to understand what would be cleaner about having an extra enum that is basically just a custom version of Value ?

spolu · 2023-11-16T09:26:32Z

Not really this would be much smaller right. The idea would be to go from JSON (in DB) to that type which is much more restricted as soon as you get out of the DB and have typed interactions everywhere instead of Value which from a typed perspective could be nested etc...

fontanierh · 2023-11-16T09:29:12Z

Ok so you are suggesting we add an intermediary type that is kind of a "narrowed" value and that implements toSQL ? I'm not necessarily against it, but IMO we don't gain much from it, because even if it can't be nested or can't be a type that we don't support in databases, it can still be of the wrong type for a given query so there is no type safety per se

fontanierh · 2023-11-16T09:34:34Z

having a custom type will let us handle the true/false <-> 1/0 conversion in ToSql / FromSql which is exactly where you would expect the translation

@spolu will go with the narrowed enum. I feel this one is a much stronger argument than the type safety one

spolu

Looks great.

One final thing that I think is important to figure out now rather than later.

core/src/databases/database.rs

spolu · 2023-11-16T11:49:59Z

core/src/databases/database.rs

+                            .map(|r| table_schema.schema.get_insert_params(&field_names, r))
+                            .collect::<Result<Vec<_>>>()?
+                            .iter()
+                            .map(|values| match stmt.execute(params_from_iter(values)) {


I think you want to run that at the end out of the par_iter. Requires benchmarking.

I suspect this inherently prevents parallelization because the pool will get filled with rows waiting for execution on the lock that protects that stmt.execute.

I would instead create all params in parallalel and then run the stmt.execute sequentially which is locked anyway.

Can we benchmark the two approach to convince ourselves of the best approach?

This is worth the time since this code is likely to be final

Already tried that, params_from_iter complains when it is in the par_iter. I can try again again but I think it's not possible because it needs to own the value (so we'd have to copy which I believe is worse ?).

creating and executing the smt cannot be done in the par_iter because conn cannot go through threads.

I am happy to benchmark, but I don't see how copying every single row's data can be faster than calling params from iter sequentially ?

Yet it's possible: https://github.com/dust-tt/dust/pull/2222/files#diff-bee624fbe402bae0a0c5cbaa790eda4654833e56e5482f62da6ff74d7fb253abR105

No copying involved

to clarify:

so I'd have to clone() the params (sequentially).

It works with into_iter instead of iter 🤔

is the former copying the data ? Or merely saying that from this point only the new interator can reference it ?

Ok that actually makes sense

spolu

LGTM \o/

Thanks for bearing with my feedback

fontanierh · 2023-11-16T12:50:05Z

🙌 stoked to merge

spolu · 2023-11-16T13:41:59Z

💯

fontanierh changed the base branch from main to feat/get-db-schema November 13, 2023 11:21

fontanierh force-pushed the feat/query-db branch from 915ad31 to 0cff1e8 Compare November 13, 2023 11:58

fontanierh changed the title ~~WIP Feat/query db~~ feat(structured data): query DB v0 Nov 13, 2023

fontanierh requested review from spolu and philipperolet November 13, 2023 12:05

spolu requested changes Nov 13, 2023

View reviewed changes

fontanierh force-pushed the feat/get-db-schema branch from a17b5b9 to c0fd006 Compare November 13, 2023 14:38

Base automatically changed from feat/get-db-schema to main November 13, 2023 14:55

fontanierh force-pushed the feat/query-db branch from 0cff1e8 to d1160f5 Compare November 13, 2023 14:58

fontanierh requested a review from spolu November 13, 2023 16:47

spolu previously approved these changes Nov 13, 2023

View reviewed changes

fontanierh dismissed spolu’s stale review via e6f6646 November 14, 2023 16:13

fontanierh force-pushed the feat/query-db branch 2 times, most recently from e6f6646 to 2a97361 Compare November 14, 2023 16:43

fontanierh added 9 commits November 14, 2023 17:43

feat: implement DB get schema (w/ endpoint)

c5420fb

database row list remove support for cross table

7e4633c

scaffolding

f91dd28

remove another for loop

bd61cf1

add some DSSTRUCTSTAT

4fdfc43

don't panic

6504eb8

Refactor database insertions to use prepared

0bb795c

statements

aesthetic nit

3092df0

fix rebase

ab048df

fontanierh force-pushed the feat/query-db branch from 2a97361 to ab048df Compare November 14, 2023 16:59

spoluify the code a bit

e8c33e9

fontanierh commented Nov 14, 2023

View reviewed changes

core/src/databases/database.rs Show resolved Hide resolved

fontanierh added 2 commits November 14, 2023 19:19

split in 2 functiong

9e6e7a7

simplify function in table schema

d4dd0fa

spolu reviewed Nov 15, 2023

View reviewed changes

core/src/databases/database.rs Show resolved Hide resolved

core/src/databases/database.rs Show resolved Hide resolved

fontanierh added 3 commits November 15, 2023 15:27

blazingly faster

a23754f

make everything work

5ee076b

more rustiness

75ffdbb

fontanierh commented Nov 15, 2023

View reviewed changes

core/Cargo.toml Outdated Show resolved Hide resolved

fontanierh commented Nov 15, 2023

View reviewed changes

core/src/databases/database.rs Outdated Show resolved Hide resolved

don't clone rows

d93d04c

fontanierh commented Nov 15, 2023

View reviewed changes

core/src/databases/database.rs Show resolved Hide resolved

fontanierh requested a review from spolu November 15, 2023 17:42

spolu reviewed Nov 16, 2023

View reviewed changes

fontanierh added 3 commits November 16, 2023 11:27

use narrowed enum

2ee45d9

remove serde_json extension for rusqlite

0240957

remove extra variable

cb241b1

fontanierh force-pushed the feat/query-db branch from 82f0942 to cb241b1 Compare November 16, 2023 11:35

fontanierh added 2 commits November 16, 2023 12:38

remove extraneous var

a9058c4

remove return

a8ef53a

fontanierh requested a review from spolu November 16, 2023 11:41

spolu reviewed Nov 16, 2023

View reviewed changes

call params_from_iter in par_iter

93bf9f4

spolu approved these changes Nov 16, 2023

View reviewed changes

fontanierh merged commit 82c3fe2 into main Nov 16, 2023
1 check passed

fontanierh deleted the feat/query-db branch November 16, 2023 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(structured data): query DB v0 #2499

feat(structured data): query DB v0 #2499

fontanierh commented Nov 13, 2023 •

edited

Loading

spolu left a comment

spolu left a comment

fontanierh commented Nov 13, 2023

spolu commented Nov 13, 2023

spolu left a comment

spolu left a comment

fontanierh commented Nov 16, 2023

spolu commented Nov 16, 2023

fontanierh commented Nov 16, 2023

fontanierh commented Nov 16, 2023

spolu left a comment

spolu Nov 16, 2023

spolu Nov 16, 2023

fontanierh Nov 16, 2023

spolu Nov 16, 2023

spolu Nov 16, 2023

fontanierh Nov 16, 2023

fontanierh Nov 16, 2023

fontanierh Nov 16, 2023

fontanierh Nov 16, 2023

fontanierh Nov 16, 2023

spolu left a comment

fontanierh commented Nov 16, 2023

spolu commented Nov 16, 2023

feat(structured data): query DB v0 #2499

feat(structured data): query DB v0 #2499

Conversation

fontanierh commented Nov 13, 2023 • edited Loading

spolu left a comment

Choose a reason for hiding this comment

spolu left a comment

Choose a reason for hiding this comment

fontanierh commented Nov 13, 2023

spolu commented Nov 13, 2023

spolu left a comment

Choose a reason for hiding this comment

spolu left a comment

Choose a reason for hiding this comment

fontanierh commented Nov 16, 2023

spolu commented Nov 16, 2023

fontanierh commented Nov 16, 2023

fontanierh commented Nov 16, 2023

spolu 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

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

spolu left a comment

Choose a reason for hiding this comment

fontanierh commented Nov 16, 2023

spolu commented Nov 16, 2023

fontanierh commented Nov 13, 2023 •

edited

Loading