feat: Added multi root queries to the model#84
Conversation
aasseman
left a comment
aasseman
left a comment
There was a problem hiding this comment.
Just a first pass, I'll review again tomorrow.
| hash: bytes | ||
| query: str | ||
| query_time_ms: int = 0 | ||
| timestamp: datetime = datetime.now() |
There was a problem hiding this comment.
The timestamp will end up being a constant set at the moment the program starts (when the class is created), not when a new instance of this class is created.
| model = await mrq_model_builder(subgraph, pgpool) | ||
| await set_cost_model(subgraph, model) | ||
| # Should this also be this amount of time? | ||
| await aio.sleep(args.relative_query_costs_refresh_interval) |
There was a problem hiding this comment.
This should be a different time, otherwise it would be too slow.
Also consider using random.lognormvariate: https://www.desmos.com/calculator/k3en6o5ikf
There was a problem hiding this comment.
What would be a good "default" value and apply lognormvariate to it, to give it some "randomness"?
There was a problem hiding this comment.
The values in the link should be a good start
| Stddev(query_time_ms) as stddev_time | ||
| FROM | ||
| query_logs | ||
| {query_logs_table} |
There was a problem hiding this comment.
@aasseman So this part, didn't let me to either leave it as %s and insert values or using $1, since the syntax doesn't work if the variable is alone like that.
Had to add format to it and use it this way, however this is vulnerable and SQL injections this way are a possibility, although this variable is not accesible by the user, what do you think?
Should I then create a separate query so that we don't have that there?
There was a problem hiding this comment.
Yeah, it's not great. There is a SQL solution mentionned here. It also mentions that psycopg2 has a solution for this, but it's not async.
However, it seems that psycopg3 came out recently, and it supports async! Not sure if it's worth/hard porting to it though.
There was a problem hiding this comment.
So looked further into this, the easiest quickest approach is to just separate the function into two or to have in the constanst file the query already defined and select the query depending the type of query its looking for , which should be the one that leaves the code the "cleanest"
Signed-off-by: carlos.vdr <carlos.vdr@semiotic.ai>
Signed-off-by: carlos.vdr <carlos.vdr@semiotic.ai>
Signed-off-by: carlos.vdr <carlos.vdr@semiotic.ai>
Signed-off-by: carlos.vdr <carlos.vdr@semiotic.ai>
Signed-off-by: carlos.vdr <carlos.vdr@semiotic.ai>
Signed-off-by: carlos.vdr <carlos.vdr@semiotic.ai>
Signed-off-by: carlos.vdr <carlos.vdr@semiotic.ai>
Signed-off-by: carlos.vdr <carlos.vdr@semiotic.ai>
Signed-off-by: carlos.vdr <carlos.vdr@semiotic.ai>
Signed-off-by: carlos.vdr <carlos.vdr@semiotic.ai>
carlosvdr
force-pushed
the
multi-root-q
branch
from
bc2b0bb to
7997689
Compare
Signed-off-by: carlos.vdr <carlos.vdr@semiotic.ai>
Pull Request Test Coverage Report for Build 5686688707Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
1 similar comment
Pull Request Test Coverage Report for Build 5686688707Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
3 participants
Issue: #43