Skip to content
This repository has been 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
merged 4 commits into from
Nov 9, 2023

Conversation

clokep
Copy link
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
retcols=(
"transaction_id",
"origin",
"ts",
"response_code",
"response_json",
"has_been_referenced",
),
retcols=("response_code", "response_json"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Optional[Tuple[bool, str, bool]],
await self.db_pool.simple_select_one(
Copy link
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
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
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
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
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
Member Author

clokep commented Nov 9, 2023

Rebased after merge of #16611.

@clokep clokep merged commit ab3f1b3 into develop Nov 9, 2023
40 checks passed
@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