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

Metadata table scans as streams #870

Merged
merged 5 commits into from
Jan 8, 2025
Merged

Conversation

rshkv
Copy link
Contributor

@rshkv rshkv commented Jan 3, 2025

This changes the metadata table APIs to have scan() return streams instead of a single RecordBatch.

Context for this is #863 (comment), where we're adding a metadata table for which we need to read multiple files. @liurenjie1024 suggested we better consume those as stream.

I'm splitting out to discuss the API change separately, even though the existing metadata tables snapshots and manifests only read a single file we're just returning async streams of a single record batch.

cc @liurenjie1024 @Xuanwo

@rshkv rshkv force-pushed the wr/metadata-async branch 2 times, most recently from 9c6514d to 359bbf2 Compare January 3, 2025 14:49
@liurenjie1024
Copy link
Contributor

Thanks @rshkv for this pr, exactly what I mean! We should rebase this pr after #872 ?

@rshkv rshkv force-pushed the wr/metadata-async branch 2 times, most recently from 5960673 to 0cf2b36 Compare January 7, 2025 10:59
@rshkv rshkv force-pushed the wr/metadata-async branch from 0cf2b36 to 27a5068 Compare January 7, 2025 11:02
@rshkv
Copy link
Contributor Author

rshkv commented Jan 7, 2025

@liurenjie1024, rebased.

We don't strictly need this before merging #863. But would be nice to agree on some stylistic "patterns" for async streams (e.g. using try_stream!, having associated per-batch functions or not).

@rshkv rshkv force-pushed the wr/metadata-async branch from d504fc9 to 8283288 Compare January 7, 2025 11:35
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @rshkv for working on this, let's move.

@Xuanwo Xuanwo merged commit 6e07faa into apache:main Jan 8, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants