feat(cubesql): CubeScan - don't clone strings for non stream response #8633
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 8 Skipped Deployments
|
| Ok(match value { | ||
| Value::String(s) => FieldValue::String(s), | ||
| Value::String(s) => FieldValue::StringRef(s), | ||
| Value::Number(n) => FieldValue::Number(n.as_f64().ok_or( |
There was a problem hiding this comment.
There is an additional boost here (we don't clone it), while n is used as a reference.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8633 +/- ##
==========================================
- Coverage 82.50% 82.50% -0.01%
==========================================
Files 209 209
Lines 76711 76718 +7
==========================================
+ Hits 63293 63298 +5
- Misses 13418 13420 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| String(String), | ||
| // Best variant for us with strings, because we dont need to clone string | ||
| StringRef(&'a String), |
There was a problem hiding this comment.
Why not reference to str? I don't think reference to String gets us anything functionally, but ties to specific allocation pattern.
There was a problem hiding this comment.
'str cannot be used because I combine branches via |. In Rust, it is a requirement to align them to the same type for usage.
| @@ -446,8 +446,11 @@ struct CubeScanExecutionPlan { | |||
| } | |||
|
|
|||
| #[derive(Debug)] | |||
| pub enum FieldValue { | |||
| pub enum FieldValue<'a> { | |||
There was a problem hiding this comment.
Add meaningful name for lifetime, like 'str/`'string' please
There was a problem hiding this comment.
I believe it's pretty much the convention to have it as 'a at least when you only have one lifetime to care about. It's pretty easy to follow with one lifetime.
There was a problem hiding this comment.
With Cow<'str, str> is not an option because it's meme
| index: usize, | ||
| field_name: &str, | ||
| ) -> std::result::Result<FieldValue, CubeError> { | ||
| let option = self.rows[index].as_object_mut(); | ||
| let as_object = if let Some(as_object) = option { | ||
| let as_object = if let Some(as_object) = self.rows[index].as_object() { |
There was a problem hiding this comment.
Can be represented as let Some(as_object) = ... else {}
|
Given that we now have both owned and borrowed variants in |
| @@ -446,8 +446,11 @@ struct CubeScanExecutionPlan { | |||
| } | |||
|
|
|||
| #[derive(Debug)] | |||
| pub enum FieldValue { | |||
| pub enum FieldValue<'a> { | |||
There was a problem hiding this comment.
I believe it's pretty much the convention to have it as 'a at least when you only have one lifetime to care about. It's pretty easy to follow with one lifetime.
ovr
force-pushed
the
feat/speedup-scan-non-stream
branch
from
d06a7b9
to
21103c1
Compare
FieldValue is used as an abstraction between transport results and building columnar data for data fusion. We don't need to clone strings because they are used as a reference for the arrow's StringBuilder. It uses its own slice under the hood.