-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
AirbyteLib: Add len() support on SQL datasets and Mapping behaviors for ReadResult (#34763) #34763
Conversation
aaronsteers
commented
Feb 2, 2024
- Make ReadResult inherit from Mapping[str, CachedDataset], including support for .keys() and .len().
- Add len() supports on datasets.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@flash1293 - This responds to some beta user feedback:
|
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.
Makes sense as convenience functions - I'm worried a little bit about len being called unexpectedly by something on a lazy dataset and taking a long time to finish, but it's probably OK as it's consistent.
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.
Actually, there are some problems with the implementation, left notes.
Yeah - I ran into the same concern upfront. But on reflecting more, if anything is explicitly requesting the num items, I think its okay to count them. 🤷 There are plenty of small sources where this is a reasonable thing to do. And for larger sources, it seems the user would be aware of the issue. The reason I'd revert is if we find there's some implicit or unexpected call to len() that the user would not anticipate. |