Skip to content

Conversation

@Alex-PLACET
Copy link
Member

No description provided.

@Alex-PLACET Alex-PLACET marked this pull request as ready for review October 15, 2025 12:50
@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@e36e33e). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/deserialize.cpp 90.00% 1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #31   +/-   ##
=======================================
  Coverage        ?   75.30%           
=======================================
  Files           ?       31           
  Lines           ?     1300           
  Branches        ?        0           
=======================================
  Hits            ?      979           
  Misses          ?      321           
  Partials        ?        0           
Flag Coverage Δ
unittests 75.30% <94.11%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

int_values.reserve(num_rows);
for (size_t i = 0; i < num_rows; ++i)
{
int_values.push_back(int_dist(gen));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use std::generate here instead of the loop.
Same for other vectors.

Copy link
Member

Choose a reason for hiding this comment

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

+1. And the distribution could be directly instantiated as the second parameter of the generate function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

{
field_names.emplace_back(field->name()->str());
}
else {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
else {
else
{

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

[[nodiscard]] ::flatbuffers::Offset<
::flatbuffers::Vector<::flatbuffers::Offset<org::apache::arrow::flatbuf::Field>>>
create_children(flatbuffers::FlatBufferBuilder& builder, sparrow::record_batch::column_range columns);
create_children(flatbuffers::FlatBufferBuilder& builder, const sparrow::record_batch& record_batch);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this redundant with the previous function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

* Helper function to create a record batch with the same schema but random values
* All batches have: int32 column, float column, bool column, and string column
*/
sp::record_batch create_random_record_batch(size_t num_rows)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move the helper functions to another file to improve readability?

Copy link
Member

Choose a reason for hiding this comment

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

I think moving them into a "detail" or "util" namespace should be enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed


int main()
{
std::cout << "=== Sparrow IPC Stream Write and Read Example ===\n";
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I missed it but it seems we're missing reading from an already existing stream file? (from the integration tests for example?) and also maybe write into a stream file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

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

I think the different "steps" of the main functions could be moved in dedicated functions so that it's easier for the reader to browse them and understand quickly each of them.

int_values.reserve(num_rows);
for (size_t i = 0; i < num_rows; ++i)
{
int_values.push_back(int_dist(gen));
Copy link
Member

Choose a reason for hiding this comment

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

+1. And the distribution could be directly instantiated as the second parameter of the generate function.

* Helper function to create a record batch with the same schema but random values
* All batches have: int32 column, float column, bool column, and string column
*/
sp::record_batch create_random_record_batch(size_t num_rows)
Copy link
Member

Choose a reason for hiding this comment

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

I think moving them into a "detail" or "util" namespace should be enough.

@JohanMabille JohanMabille merged commit 04433e7 into QuantStack:main Oct 16, 2025
26 of 27 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.

4 participants