Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Improve type annotations for execute_values. #12311

Merged
merged 6 commits into from
Mar 28, 2022
Merged

Improve type annotations for execute_values. #12311

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
252a71e
Add note about how we could use ParamSpec in _do_execute
reivilibre Mar 28, 2022
31b36c9
Add a narrower type annotation for execute_values's `args` parameter
reivilibre Mar 28, 2022
0732a86
`values` is a better name than `args`
reivilibre Mar 28, 2022
e5785b1
Newsfile
reivilibre Mar 28, 2022
470d309
Revert "Add note about how we could use ParamSpec in _do_execute"
reivilibre Mar 28, 2022
8d58994
Don't thread args through _do_execute needlessly
reivilibre Mar 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/12311.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improve type annotations for `execute_values`.
26 changes: 17 additions & 9 deletions synapse/storage/database.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,9 @@ def execute_batch(self, sql: str, args: Iterable[Iterable[Any]]) -> None:
else:
self.executemany(sql, args)

def execute_values(self, sql: str, *args: Any, fetch: bool = True) -> List[Tuple]:
def execute_values(
self, sql: str, values: Iterable[Iterable[Any]], fetch: bool = True
) -> List[Tuple]:
"""Corresponds to psycopg2.extras.execute_values. Only available when
using postgres.

Expand All @@ -305,15 +307,11 @@ def execute_values(self, sql: str, *args: Any, fetch: bool = True) -> List[Tuple
from psycopg2.extras import execute_values

return self._do_execute(
# Type ignore: mypy is unhappy because if `x` is a 5-tuple, then there will
# be two values for `fetch`: one given positionally, and another given
# as a keyword argument. We might be able to fix this by
# - propagating the signature of psycopg2.extras.execute_values to this
# function, or
# - changing `*args: Any` to `values: T` for some appropriate T.
lambda *x: execute_values(self.txn, *x, fetch=fetch), # type: ignore[misc]
lambda the_sql, the_values: execute_values(
self.txn, the_sql, the_values, fetch=fetch
Copy link
Member

Choose a reason for hiding this comment

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

(This code was pre-existing, but...)

the_sql doesn't change for each iterate, correct? So really no reason to pass it in for each one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, what do you mean?

the_sql is given by _do_execute.

the_values are just a pass-through of values and could have been part of the lambda's closure, but in analogue to execute_batch (and indeed the way this code was written before), it's passed through. If we want to change this I suppose I have no objections, though I wonder if the idea was that _do_execute could have transformed the args if necessary...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Maybe the confusion here is that there's no iteration here: it's a one-off callback)

Copy link
Member

Choose a reason for hiding this comment

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

Errr, yes...that didn't make sense.

My confusion is -- why are we passing in fetch directly into the lambda, but we pass in the_sql and the_values as arguments to the lambda?

Copy link
Contributor Author

@reivilibre reivilibre Mar 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

the_sql is not something we know, since _do_execute computes it.

the_values is something we indeed could pass straight in.

I wonder if the reason is historical: perhaps fetch = True (a keyword argument) didn't come until later; perhaps the code used to look like:

_do_execute(self.txn.execute_values, sql, values)

EDIT: this seems likely because:

    def execute(self, sql: str, *args: Any) -> None:
        self._do_execute(self.txn.execute, sql, *args)

    def executemany(self, sql: str, *args: Any) -> None:
        self._do_execute(self.txn.executemany, sql, *args)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe _do_execute should just allow kwargs instead? I think that would be cleaner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, nevermind. execute_values isn't a method, so it'd need a lambda to shuffle the parameters around anyway

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I did not realize that _do_execute modified the passed in sql!

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've changed this around a bit, is that better to you?

Copy link
Member

Choose a reason for hiding this comment

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

I do find that clearer, yes!

),
sql,
*args,
values,
)

def execute(self, sql: str, *args: Any) -> None:
Expand All @@ -326,6 +324,16 @@ def _make_sql_one_line(self, sql: str) -> str:
"Strip newlines out of SQL so that the loggers in the DB are on one line"
return " ".join(line.strip() for line in sql.splitlines() if line.strip())

# TODO(ParamSpec):
# Once Mypy supports Concatenate, this could be written as
# def _do_execute(
# self,
# func: Callable[Concatenate[str, P], R],
# sql: str,
# *args: P.args,
# **kwargs: P.kwargs,
# ) -> R:
# so long as we also pass kwargs.
reivilibre marked this conversation as resolved.
Show resolved Hide resolved
def _do_execute(self, func: Callable[..., R], sql: str, *args: Any) -> R:
sql = self._make_sql_one_line(sql)

Expand Down