Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

feat: use istziio_client trait for benchmarking #53

Merged
merged 5 commits into from
Apr 29, 2024
Merged

Conversation

xx01cyx
Copy link
Member

@xx01cyx xx01cyx commented Apr 28, 2024

No description provided.

@xx01cyx xx01cyx changed the title benchmark: use istziio_client trait for benchmarking feat: use istziio_client trait for benchmarking Apr 28, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 87.16%. Comparing base (dfd706d) to head (0a5a250).

Files Patch % Lines
storage-client/src/client.rs 62.50% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
- Coverage   87.36%   87.16%   -0.20%     
==========================================
  Files          18       18              
  Lines        2263     2259       -4     
  Branches     2263     2259       -4     
==========================================
- Hits         1977     1969       -8     
+ Misses        172      171       -1     
- Partials      114      119       +5     

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

@xx01cyx xx01cyx requested a review from unw9527 April 28, 2024 06:42
storage-client/src/client.rs Outdated Show resolved Hide resolved
@@ -1,7 +1,8 @@
use arrow_array::Float64Array;
use arrow::array::Float64Array;
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe in arrow 50.0.0 we should use arrow::array instead of arrow_array

use hyper::Uri;
use lazy_static::lazy_static;
use parquet::arrow::{ParquetRecordBatchStreamBuilder, ProjectionMask};
use parquet::arrow::arrow_reader::ParquetRecordBatchReaderBuilder;
Copy link
Member

Choose a reason for hiding this comment

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

Difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have to switch to arrow 50.0.0 since istziio_client uses 50.0.0. And we also need to use arrow::record_batch::RecordBatch since istziio_client uses this one. Some changes are made accordingly including this.

.unwrap()
.with_batch_size(BATCH_SIZE);

let file = File::open(file_path)?;
Copy link
Member

Choose a reason for hiding this comment

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

Why not async open?

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

@@ -1,7 +1,7 @@
pub mod client;

use anyhow::Result;
use arrow_array::RecordBatch;
use arrow::record_batch::RecordBatch;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔️ sounds reasonable but istziio_client already uses arrow::record_batch::RecordBatch. Shall we ask them to modify the interface?

Copy link
Member

Choose a reason for hiding this comment

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

Could you talk to them about this first? We can discuss other things later tmr.

@unw9527
Copy link
Member

unw9527 commented Apr 28, 2024

Please change the batch size. Other than this, LGTM.

@xx01cyx xx01cyx merged commit 94b59cc into main Apr 29, 2024
1 check passed
@unw9527 unw9527 deleted the yx/client-trait branch April 30, 2024 00:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants