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

Convert simple_select_one_txn and simple_select_one to return tuples.#16612

Merged
clokep merged 4 commits into
developfrom
clokep/axe-dicts
Nov 9, 2023
Merged

Convert simple_select_one_txn and simple_select_one to return tuples.#16612
clokep merged 4 commits into
developfrom
clokep/axe-dicts

Conversation

@clokep
Copy link
Copy Markdown
Member

@clokep clokep commented Nov 8, 2023

This is the last simple_* API that automatically creates dictionaries of the provided columns.

The vast majority of the time we return only a couple of columns and immediately unpack the dictionary. This is wasteful and a continuation of #16431.

Builds on #16611.

@clokep clokep force-pushed the clokep/axe-dicts branch 2 times, most recently from c4b714e to 2ffd6c0 Compare November 8, 2023 21:03
@clokep clokep marked this pull request as ready for review November 8, 2023 21:14
@clokep clokep requested a review from a team as a code owner November 8, 2023 21:14
"response_json",
"has_been_referenced",
),
retcols=("response_code", "response_json"),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!

Comment thread synapse/storage/databases/main/room.py Outdated
Optional[Tuple[bool, str, bool]],
await self.db_pool.simple_select_one(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this going to give us integers on sqlite rather than bools?

Copy link
Copy Markdown
Contributor

@DMRobertson DMRobertson Nov 9, 2023

Choose a reason for hiding this comment

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

Also: the boolean fieldsall three fields seem to be nullable in the DB. Sorry if there is already an issue for this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Filed #16614.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is this going to give us integers on sqlite rather than bools?

Luckily in this case the we can just do bool(...) to handle both ints and nulls. Turns out we don't ever use the creator, so I dropped it from the response.

Copy link
Copy Markdown
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

No other nitpicks!

Base automatically changed from clokep/media-attr to develop November 9, 2023 16:00
@clokep
Copy link
Copy Markdown
Member Author

clokep commented Nov 9, 2023

Rebased after merge of #16611.

@clokep clokep merged commit ab3f1b3 into develop Nov 9, 2023
@clokep clokep deleted the clokep/axe-dicts branch November 9, 2023 16:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants