Skip to content

feat: Allow read from stream #4906

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.

Sign up for GitHub

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

Jump to bottom
Merged
joseph-isaacs merged 5 commits into from
Oct 10, 2025
Merged

feat: Allow read from stream #4906

joseph-isaacs merged 5 commits into from
Oct 10, 2025

Conversation

@m7kss1
Copy link
Contributor

@m7kss1 m7kss1 commented Oct 9, 2025 •
edited
Loading

No description provided.

@m7kss1 m7kss1 changed the title [CXX] Allow read from stream [WIP] Allow read from stream Oct 9, 2025
@m7kss1 m7kss1 changed the title [WIP] Allow read from stream [CXX] Allow read from stream Oct 9, 2025
@joseph-isaacs joseph-isaacs changed the title [CXX] Allow read from stream feat[CXX]: Allow read from stream Oct 9, 2025
@joseph-isaacs joseph-isaacs added the changelog/feature A new feature label Oct 9, 2025
@joseph-isaacs
Copy link
Contributor

joseph-isaacs commented Oct 9, 2025

Mind signing the commits to make (DCO https://github.com/vortex-data/vortex/pull/4906/checks?check_run_id=52383374445) happy

@m7kss1 m7kss1 changed the title feat[CXX]: Allow read from stream feat: Allow read from stream Oct 9, 2025
@joseph-isaacs
Copy link
Contributor

joseph-isaacs commented Oct 9, 2025

We plan on in the future using vortex-ipc to stream serialized vortex arrays, however for now we don't have so I think adding a way to create a VortexFile using a byte array seems reasonable.

@m7kss1
DCO Remediation Commit for Dergousov Maxim <dergousovmaxim99@gmail.com>
8e25958 
I, Dergousov Maxim <dergousovmaxim99@gmail.com>, hereby add my Signed-off-by to this commit: 928c8b1

Signed-off-by: Dergousov Maxim <dergousovmaxim99@gmail.com>
@codecov
Copy link

codecov bot commented Oct 9, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.40%. Comparing base (be66523) to head (6bff9c3).
⚠️ Report is 5 commits behind head on develop.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

0ax1
0ax1 reviewed Oct 10, 2025
View reviewed changes
vortex-cxx/cpp/src/file.cpp

namespace vortex {

VortexFile VortexFile::Open(const uint8_t *data, size_t length) {
Copy link
Contributor

@0ax1 0ax1 Oct 10, 2025

Choose a reason for hiding this comment

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

std::span would be preferable here, though we currently compile with C++17 (span was introduced with C++20). @joseph-isaacs We should discuss bumping to C++20 or considering a library like abseil in order to use span. This is of course not in scope of this PR.

@m7kss1
fix: lint check
d19e9af 
Signed-off-by: Dergousov Maxim <dergousovmaxim99@gmail.com>
@m7kss1 m7kss1 force-pushed the cxx/allow_open_stream branch from 50b53f2 to d19e9af Compare October 10, 2025 10:45
joseph-isaacs
joseph-isaacs approved these changes Oct 10, 2025
View reviewed changes
Copy link
Contributor

@joseph-isaacs joseph-isaacs left a comment

Choose a reason for hiding this comment

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

Thanks for contribution

@joseph-isaacs joseph-isaacs merged commit 23cefb0 into vortex-data:develop Oct 10, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0ax1 0ax1 0ax1 left review comments

@joseph-isaacs joseph-isaacs joseph-isaacs approved these changes

Assignees

No one assigned

Labels

changelog/feature A new feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@m7kss1 @joseph-isaacs @0ax1