-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Convert simple_select_many_batch, simple_select_many_txn to tuples. #16444
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks sane and would like to see it land. I mostly ended up double-checking the casts. I think in a few places we don't say Optional[] where the corresponding DB column is nullable---would you mind fixing those up?
# We sort the rows so that the most recently added entry is picked up. | ||
rows.sort(key=lambda r: r["ts_added_ms"]) | ||
# We sort the rows by ts_added_ms so that the most recently added entry | ||
# will stomp over older entries in the dictionary. | ||
rows.sort(key=lambda r: r[2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhhhh I see: key ids are not unique in this table.
We could probably get the DB to do that for us (e.g. https://stackoverflow.com/questions/13325583/postgresql-max-and-group-by) , but I doubt it makes much difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is annoying to do for both sqlite & postgres, I think. And I suspect Erik was attempting to not change too much code? Not 100% sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working through this!
🪓
cursor_to_dict
, more of #16431.This one got a bit big, but I couldn't really figure out a good way to separate them. It is fairly tedious, but I'll try to mark noticeable spots.