Skip to content
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

Generalize data access functions #67

Closed
dmpetrov opened this issue Jul 17, 2024 · 6 comments · Fixed by #121
Closed

Generalize data access functions #67

dmpetrov opened this issue Jul 17, 2024 · 6 comments · Fixed by #121
Assignees

Comments

@dmpetrov
Copy link
Member

dmpetrov commented Jul 17, 2024

[Based on several discussions with @volkfox & @skshetry]

collect()/collect_one() needs to be combined - return single-value-list when only one signal is requested
iterate()/iterate_one() - the same

@ilongin ilongin self-assigned this Jul 17, 2024
@rlamy
Copy link
Contributor

rlamy commented Jul 17, 2024

BTW, I'm not sure why we have both iterate() and collect(), since it's trivial and common to turn an iterator into a list (see e.g. usage patterns for built-in map()). It also leads users to inefficient code like for x in chain.collect():.
My suggestion: remove collect() and rename iterate() to collect().

@skshetry
Copy link
Member

So, it looks like we'll have two APIs: collect() and collect_flatten().

@skshetry
Copy link
Member

But we need to decide what to do with results() and to_records(). They are very similar to collect_flatten.

@dmpetrov
Copy link
Member Author

if results() == collect_flatten() then, yes, let's rename it to collect_flatten.

to_records() does not look similar since it returns json of flatten fields. I'm not sure this is needed in user API level / DC. We can keep in DatasetQuery level just in case or even rename it to _to_records.

@dmpetrov
Copy link
Member Author

hey folks, does anyone working on this? This is P1 - we need it ASAP. Like till Monday morning.

@dmpetrov
Copy link
Member Author

@ilongin are you working on it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants