chroma icon indicating copy to clipboard operation
chroma copied to clipboard

Use strings everywhere, not UUID + add more types

Open AugustKarlstedt opened this issue 2 years ago • 3 comments

Description of changes

Adds a bunch of types which (i think) are correct ~ pls lmk if not. Also this should fix #126 by using Strings everywhere, even in Clickhouse where they have a uuid type. Think this makes sense since string uuids are more universally compatible with other possible future backends.

Summarize the changes made by this PR.

  • Improvements & Bug fixes
    • Adds a bunch of typing
    • Removes use of UUID type and uses raw strings
  • New functionality
    • ...

Test plan

How are these changes tested? Current tests

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository? Potentially

AugustKarlstedt avatar Feb 24 '23 05:02 AugustKarlstedt

Hi August, appreciate all the changes! I think we want to keep our use of uuids as UUIDs internally to the databases and when they're passed around. Reason being:

  1. We want to be as storage sensitive as possible and the DBs here store uuids more efficiently when using their built in uuid type as opposed to strings (128bit int vs the hexadecimal string)
  2. The affordances of the UUID class are nice for users (.bytes / .hex) etc.

Not that hung up on 2nd point as you could always wrap it if you wanted.

Would you be able to change this to make it so that we use UUID over str?

HammadB avatar Mar 03 '23 18:03 HammadB

@AugustKarlstedt we would love to get this merged! would it be ok with you if I hijack it and remove the UUID->string change for now?

jeffchuber avatar Mar 17 '23 05:03 jeffchuber

hey @jeffchuber, totally cool w/ me!

AugustKarlstedt avatar Mar 20 '23 23:03 AugustKarlstedt

@AugustKarlstedt this PR got pulled into the large backend refactor that @levand and @HammadB are working on. thanks for guiding us through it with this PR. Changes will be integrated with that PR when it lands.

jeffchuber avatar Apr 22 '23 01:04 jeffchuber