SVLS-6336 fix: support tag that are not key-value pairs #826
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BenchmarksComparisonBenchmark execution time: 2025-01-27 20:57:37 Comparing candidate commit 8b06f81 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 2 unstable metrics. CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
BaselineOmitted due to size. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #826 +/- ##
==========================================
- Coverage 71.37% 71.37% -0.01%
==========================================
Files 317 317
Lines 46605 46637 +32
==========================================
+ Hits 33263 33285 +22
- Misses 13342 13352 +10
|
duncanista
approved these changes
Jan 24, 2025
pawelchcki
reviewed
Jan 27, 2025
pawelchcki
approved these changes
Jan 27, 2025
tobz
approved these changes
Jan 27, 2025
| @@ -89,15 +88,23 @@ impl SortedTags { | |||
| pub fn to_chars(&self) -> Vec<Chars> { | |||
| let mut tags_as_chars = Vec::new(); | |||
| for (k, v) in &self.values { | |||
| tags_as_chars.push(format!("{}:{}", k, v).into()); | |||
| if v.is_empty() { | |||
| tags_as_chars.push(Chars::from(k.to_string())); | |||
There was a problem hiding this comment.
For bare tags, you can optimize this a little further:
Ustr::as_str(the type ofk) gives you a&'static strBytes::from_staticcan be constructed from&'static [u8], which you can get from the&'static strby calling.to_bytes()- you can then jump from
BytestoCharsviaChars::from_bytes(which does return aResult<T, E>but should be safe to call.expecton since the safety condition it's validating is that you passed in valid UTF-8, which we know we did since we got ourBytesdirectly from a&str, which should always be valid UTF-8`
The end result is that we'll always skip the allocation for bare tags, and the only cost we'll pay is the UTF-8 validation, which is minimal.
There was a problem hiding this comment.
This second optimization seems a bit less readable, add the expect path and requires an extra dependency for Bytes::from_static, so I would just use a pre allocated string as in the similar code too, unless there is a strong feeling about optimizing this
alexgallotta
force-pushed
the
support-keyonly-tags
branch
from
77a88e9 to
bd0027f
Compare
paullegranddc
pushed a commit
that referenced
this pull request
Feb 3, 2025
* fix: support tag that are not key-value pairs * fix comment * add test for tag with multiple columns * use more efficient string concatenation * clippy
alexgallotta
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow tags that are not a pair key:value to be passed. Use an empty string as value and do not serialize a column with empty string in that case