-
Notifications
You must be signed in to change notification settings - Fork 20
feat: add document loader #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
12cfd97 to
96b9b00
Compare
09172ac to
6706a5d
Compare
7188997 to
78d1aab
Compare
10c2257 to
9b7b9c4
Compare
169a5ea to
8315d9d
Compare
| if type(self.staleness) is float | ||
| else None | ||
| ) | ||
| snapshot = db.batch_snapshot(exact_staleness=duration, read_timestamp=timestamp) |
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.
why we are passing both? we can just check and pass, right?
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.
One or the other should be "None", therefore simplifying the code by not needing conditionals.
| if type(self.staleness) is float | ||
| else None | ||
| ) | ||
| snapshot = db.batch_snapshot(exact_staleness=duration, read_timestamp=timestamp) |
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.
We should support all the four options:
- read_timestamp: Optional[datetime.datetime] = None,
- min_read_timestamp: Optional[datetime.datetime] = None,
- max_staleness: Optional[datetime.timedelta] = None,
- exact_staleness: Optional[datetime.timedelta] = None,
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.
The design was scoped to just read_timestamp and exact_staleness
| ) | ||
| for doc in documents | ||
| ] | ||
| keys = [[doc[0]] for doc in docs] |
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 guess it won't work if the primary key is composite?
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.
Added another argument in DocumentSaver -- primary key. and used that to build the self._table_fields in the init method. The self._table_fields will be built such as the primary key is always the first in the list. This will ensure that the first field in the tuple will always be the primary key. Thank you for the review!
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕