feat(clp-s): Unescape string values during ingestion and fix support for search using escape sequences. #622
Conversation
WalkthroughThis pull request introduces comprehensive changes to string handling, token processing, and escape sequence management across multiple components in the CLP (Compressed Log Processing) system. The modifications primarily focus on enhancing string escaping capabilities, improving Unicode and KQL token processing, and refining column descriptor creation. Key areas of change include utility functions for JSON and KQL string handling, column reader enhancements, and updates to parsing and serialization mechanisms. Changes
Possibly Related PRs
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
gibber9809
changed the title
gibber9809
force-pushed
the
escape-char-fix
branch
from
7871210 to
0b71b46
Compare
gibber9809
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
components/core/src/clp_s/JsonParser.cpp (2)
Line range hint
245-275: Consider refactoring timestamp matching logicThe timestamp matching logic contains multiple boolean flags and a complex state machine. This could be simplified for better maintainability.
Consider extracting the timestamp matching logic into a separate method:
+private: + struct TimestampMatchState { + bool can_match = false; + bool may_match = false; + bool matches = false; + int longest_matching_prefix = 0; + }; + + TimestampMatchState update_timestamp_match( + const TimestampMatchState& state, + const std::string_view& key, + size_t depth) { + TimestampMatchState new_state = state; + if (!new_state.can_match) return new_state; + + if (new_state.may_match) { + if (depth <= m_timestamp_column.size() + && key == m_timestamp_column[depth - 1]) { + new_state.matches = (depth == m_timestamp_column.size()); + } else { + new_state.longest_matching_prefix = depth - 1; + new_state.may_match = false; + } + } + return new_state; + }
Line range hint
352-377: Consider consolidating string type determination logicThe logic for determining whether to use
ClpStringorVarStringbased on space character presence is repeated in multiple places. This could be extracted into a helper method.+private: + NodeType determine_string_type(std::string_view value) const { + return value.find(' ') != std::string::npos + ? NodeType::ClpString + : NodeType::VarString; + }components/core/src/clp_s/Utils.hpp (1)
308-308: Fix typo in documentationThere is a typo in the word "succesfully" in the documentation for
unescape_kql_internal. It should be "successfully."Apply this diff to correct the typo:
- * @return true if the value was unescaped succesfully and false otherwise. + * @return true if the value was unescaped successfully, false otherwise.components/core/src/clp_s/search/SchemaMatch.cpp (1)
80-88: Consider inserting descriptors at the beginning to avoid reversingCurrently, descriptors are added to the vector and then reversed. Inserting descriptors at the beginning can improve performance by eliminating the need for the reverse operation.
Apply this diff to modify the code:
- descriptors.emplace_back( + descriptors.insert(descriptors.begin(), DescriptorToken::create_descriptor_from_literal_token( node->get_key_name() ) );Remove the reverse operation:
- std::reverse(descriptors.begin(), descriptors.end());components/core/src/clp_s/ColumnReader.cpp (1)
92-105: Consider optimizing escaping by performing it during decodingThe
extract_escaped_string_value_into_buffermethod could be optimized by escaping strings during decoding, as suggested by the TODO comment. This change could improve performance.Would you like assistance in implementing this optimization or opening a GitHub issue to track it?
components/core/src/clp_s/search/ColumnDescriptor.hpp (1)
71-99: Consider providing a more descriptive error messageWhen throwing
OperationFaileddue to an incomplete escape sequence, including the problematic token or additional context in the exception message would aid in debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
components/core/src/clp_s/ColumnReader.cpp(3 hunks)components/core/src/clp_s/ColumnReader.hpp(3 hunks)components/core/src/clp_s/JsonParser.cpp(7 hunks)components/core/src/clp_s/JsonSerializer.hpp(5 hunks)components/core/src/clp_s/Utils.cpp(2 hunks)components/core/src/clp_s/Utils.hpp(3 hunks)components/core/src/clp_s/clp-s.cpp(1 hunks)components/core/src/clp_s/search/AddTimestampConditions.cpp(1 hunks)components/core/src/clp_s/search/ColumnDescriptor.cpp(4 hunks)components/core/src/clp_s/search/ColumnDescriptor.hpp(4 hunks)components/core/src/clp_s/search/SchemaMatch.cpp(1 hunks)components/core/src/clp_s/search/kql/Kql.g4(2 hunks)components/core/src/clp_s/search/kql/kql.cpp(8 hunks)components/core/tests/test-kql.cpp(7 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
components/core/src/clp_s/JsonParser.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/search/AddTimestampConditions.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/clp-s.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/ColumnReader.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/tests/test-kql.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/ColumnReader.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/Utils.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/JsonSerializer.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/Utils.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/search/SchemaMatch.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/search/ColumnDescriptor.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/search/ColumnDescriptor.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/search/kql/kql.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🔇 Additional comments (38)
components/core/src/clp_s/search/AddTimestampConditions.cpp (3)
Line range hint 10-11: Excellent use of boolean expression style!
The code correctly follows the guideline of using false == <expression> rather than !<expression>.
Line range hint 17-18: Consistent boolean expression style maintained!
The code maintains consistency with the preferred false == <expression> pattern.
21-22: Verify consistent usage of new descriptor creation method
The change to create_from_escaped_tokens aligns with the PR's objective of improving string value handling. However, let's verify consistent usage across the codebase.
✅ Verification successful
Change to create_from_escaped_tokens is correctly implemented and used consistently
The verification shows that:
- The new method
create_from_escaped_tokensis used consistently across all relevant files - No instances of the old
createmethod remain in the codebase - The header file confirms this is the intended API, with clear documentation and proper implementation of string escaping functionality
- The change is used in all appropriate contexts: clp-s.cpp, kql.cpp, and AddTimestampConditions.cpp
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of new method and identify any remaining old usage
# Look for both old and new method calls to ensure complete migration
echo "Checking for new method usage:"
rg "ColumnDescriptor::create_from_escaped_tokens" -A 2
echo "Checking for potential remaining old method usage:"
rg "ColumnDescriptor::create\(" -A 2
# Check for any TODO or FIXME comments related to this change
rg -i "todo.*columndescriptor|fixme.*columndescriptor"
Length of output: 1428
Script:
#!/bin/bash
# Let's verify if there are any other methods in ColumnDescriptor class
# and check their implementation to understand the complete context
echo "Checking ColumnDescriptor class implementation:"
ast-grep --pattern 'class ColumnDescriptor {
$$$
}'
echo "Checking for any tests using these methods:"
rg -l "ColumnDescriptor.*create" "*test*"
echo "Checking header file for method declarations:"
fd "ColumnDescriptor.h" --exec cat {}
Length of output: 10376
components/core/src/clp_s/clp-s.cpp (1)
209-209: Update to create_from_escaped_tokens enhances escaped token handling
The change to use ColumnDescriptor::create_from_escaped_tokens ensures that column descriptors are correctly created from escaped tokens, aligning with the improved handling of escaped strings throughout the codebase.
components/core/src/clp_s/JsonParser.cpp (3)
66-66: Efficient use of std::string_view for key handling
The change from std::string to std::string_view for cur_key and direct assignment from unescaped_key(true) is a good optimization that avoids unnecessary string copies.
Also applies to: 84-84
134-134: Verify error handling for malformed escape sequences
With the changes to use get_string(true) for unescaped strings, we should verify proper error handling for malformed escape sequences.
Also applies to: 210-210, 352-352
✅ Verification successful
Let me gather more context about the error handling in this file.
Let me check one more aspect about the error handling specifically around the string parsing.
Let me try one last search with corrected syntax.
Based on the gathered evidence, I can now provide the final response:
Error handling for malformed escape sequences is managed at a higher level
The error handling for malformed escape sequences is already handled by the simdjson library at the document parsing level through JsonFileIterator, which captures UTF8 errors and IO errors. The get_string(true) calls are made only after successful type checking and within the context of already validated JSON documents.
- Error handling occurs in JsonFileIterator with specific handling for
simdjson::error_code::UTF8_ERROR - The parser has top-level error handling for
simdjson::error_code::SUCCESSvalidation - String parsing is done only after type validation via
ondemand::json_type::stringchecks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling for malformed escape sequences
# Search for error handling around get_string calls
rg -B 2 -A 2 "get_string.*true"
# Search for try-catch blocks that might handle simdjson errors
ast-grep --pattern 'try {
$$$
$obj.get_string($$$)
$$$
} catch'
Length of output: 1479
Script:
#!/bin/bash
# Check for try-catch blocks around the usage of these string values
rg -B 5 -A 5 "case ondemand::json_type::string:"
# Look for any error handling related to simdjson
rg -B 2 -A 2 "simdjson::error"
# Check the class definition and member declarations for error handling
ast-grep --pattern 'class JsonParser {
$$$
}'
Length of output: 10549
Script:
#!/bin/bash
# Check for any try-catch blocks in JsonParser.cpp
rg -B 3 -A 3 "try\s*{" components/core/src/clp_s/JsonParser.cpp
# Look for string parsing related error handling
rg -B 3 -A 3 "ondemand::json_type::string.*get_string" components/core/src/clp_s/JsonParser.cpp
# Check if there's any specific string validation
rg -B 2 -A 2 "validate.*string" components/core/src/clp_s/JsonParser.cpp
Length of output: 316
Script:
#!/bin/bash
# Check for try-catch blocks with fixed regex
rg -B 3 -A 3 "try {" components/core/src/clp_s/JsonParser.cpp
# Look for error handling in the string parsing function
rg -B 3 -A 3 "get_string\(" components/core/src/clp_s/JsonParser.cpp
# Check the JsonFileIterator implementation for string handling
rg -B 3 -A 3 "parse.*string" components/core/src/clp_s/JsonFileIterator.cpp
Length of output: 1289
134-134: Verify string value lifetime management
The changes to use get_string(true) with std::string_view across multiple methods improve performance by avoiding copies. However, we should verify that the string views' lifetimes are properly managed.
Also applies to: 210-210, 352-352
✅ Verification successful
Let me analyze the string lifetime management in the codebase further by checking how the string views from get_string() are used.
Let me try a different approach to analyze the string lifetime management by checking the full context around these get_string() calls.
Let me check one more aspect to verify the simdjson string lifetime guarantees.
String lifetime management appears safe
The string_view values obtained from get_string(true) are used safely within their scope:
- Values are immediately passed to add_node() for schema building
- Values are stored in m_current_parsed_message which handles the string storage
- No string_views are stored as class members in JsonParser
- The true parameter in get_string() indicates the string memory is managed by simdjson
The current usage pattern ensures string_views don't outlive their source strings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential string lifetime issues by finding all usages of string_view
# that might outlive the source string
# Search for string_view storage in class members or containers
rg -A 3 "string_view.*;"
# Search for string_view being stored beyond the scope of get_string
ast-grep --pattern 'std::string_view $var = $obj.get_string($$$);
$$$
return $var;'
Length of output: 67062
Script:
#!/bin/bash
# Check how string_view values from get_string() are used
ast-grep --pattern 'std::string_view $var = $obj.get_string($$$);
$$$'
# Check for any string_view member variables that might store these values
rg "string_view.*;" -A 2 "JsonParser"
Length of output: 163
Script:
#!/bin/bash
# Search for get_string usage with more context
rg -A 5 -B 5 "get_string\(true\)" components/core/src/clp_s/JsonParser.cpp
# Check for any string_view storage in the JsonParser class
rg "string_view" components/core/src/clp_s/JsonParser.hpp
Length of output: 1749
Script:
#!/bin/bash
# Check simdjson documentation/headers for string lifetime guarantees
rg -A 3 "get_string.*true" components/core/src/simdjson
# Check if the string_view values are used beyond their immediate scope
rg -A 10 "value = .*get_string\(true\)" components/core/src/clp_s/JsonParser.cpp
# Look for any string storage in the archive writer
rg "add_node.*NodeType::(ClpString|VarString|DateString)" components/core/src/clp_s/
Length of output: 2889
components/core/src/clp_s/Utils.hpp (2)
4-4: Inclusion of <array> is appropriate
The addition of <array> is necessary to support the use of std::array in the code.
213-214: Documentation update enhances clarity
The updated documentation for tokenize_column_descriptor provides clearer information about its functionality related to KQL columns.
components/core/src/clp_s/Utils.cpp (5)
4-4: Including <simdjson.h> for JSON parsing is appropriate
The addition of <simdjson.h> is necessary for efficient JSON parsing.
436-485: The tokenize_column_descriptor function correctly handles escaping and tokenization
The updated logic in tokenize_column_descriptor properly manages escape sequences and token boundaries, ensuring accurate tokenization of descriptors.
486-540: The escape_json_string method efficiently escapes JSON strings
The implementation using a lambda function and unescaped slices optimizes performance when processing JSON strings.
541-569: convert_four_byte_hex_to_utf8 correctly converts hex sequences to UTF-8
The function appropriately handles Unicode conversion using simdjson, ensuring correct interpretation of Unicode escape sequences.
571-674: The unescape_kql_value and unescape_kql_internal methods correctly unescape KQL values
The methods effectively manage various escape sequences, including Unicode characters, ensuring accurate unescaping of KQL values.
components/core/src/clp_s/JsonSerializer.hpp (5)
73-77: Special keys are properly escaped before being stored
The add_special_key method ensures that keys are correctly escaped using StringUtils::escape_json_string.
119-119: append_key correctly delegates to append_escaped_key
This change ensures that keys are appended with proper escaping.
123-123: Escaping keys before appending enhances JSON correctness
Using StringUtils::escape_json_string ensures keys are correctly escaped in the JSON output.
140-140: Values are properly escaped when appended with quotes
The use of extract_escaped_string_value_into_buffer guarantees that string values are correctly escaped before being added to the JSON string.
145-149: append_escaped_key correctly appends already escaped keys
As the keys are pre-escaped, this method correctly appends them to the JSON string without additional escaping.
components/core/src/clp_s/ColumnReader.cpp (2)
5-5: Including "Utils.hpp" is appropriate for utility functions
This inclusion allows the use of StringUtils methods within the file.
143-149: VariableStringColumnReader correctly escapes string values
The method ensures that string values are properly escaped before being appended, maintaining JSON validity.
components/core/src/clp_s/ColumnReader.hpp (3)
56-65: Well-structured virtual method addition!
The new virtual method follows good design practices with clear documentation and a sensible default implementation.
166-168: Correct override declaration!
The method is properly declared with the override specifier.
213-215: Correct override declaration!
The method is properly declared with the override specifier.
components/core/tests/test-kql.cpp (5)
6-6: Appropriate include addition!
The fmt library include is correctly placed and necessary for the new formatting functionality.
69-70: Good use of the new token creation method!
Using create_descriptor_from_escaped_token improves the handling of escape sequences in descriptors.
87-88: Consistent use of the new token creation method!
The change maintains consistency with other sections in handling escaped tokens.
133-134: Consistent token creation across expression types!
The changes maintain consistency in token creation across AND and OR expressions.
Also applies to: 183-184
243-267: Excellent test coverage for escape sequences!
The new test section provides comprehensive coverage of various escape sequences, including:
- Backslashes and special characters
- Unicode sequences
- Control characters
- Special KQL characters
The test structure using input/output pairs makes the expected behaviour clear and maintainable.
components/core/src/clp_s/search/ColumnDescriptor.hpp (3)
19-24: Good addition of custom exception class for improved error handling
The introduction of the OperationFailed exception class enhances error reporting by providing more specific context when token operations fail.
32-34: Refactoring with static factory methods enhances clarity
The use of static methods create_descriptor_from_escaped_token and create_descriptor_from_literal_token improves code readability and enforces consistent creation of DescriptorToken instances.
Also applies to: 40-41
124-130: New static methods improve ColumnDescriptor instantiation
Adding create_from_escaped_token, create_from_escaped_tokens, and create_from_descriptors methods enhances clarity and ensures consistent creation of ColumnDescriptor objects.
components/core/src/clp_s/search/kql/Kql.g4 (1)
61-61: Appropriate inclusion of UNICODE in UNQUOTED_CHARACTER
Adding UNICODE to UNQUOTED_CHARACTER fragment extends the grammar to support Unicode escape sequences in unquoted literals, enhancing the language's expressiveness.
components/core/src/clp_s/search/ColumnDescriptor.cpp (2)
9-9: Use of create_descriptor_from_escaped_token improves consistency
Replacing direct instantiation with create_descriptor_from_escaped_token ensures consistent token creation and proper handling of escaped tokens throughout the codebase.
Also applies to: 27-27
52-66: Renaming and updating create methods enhances clarity
Renaming the create methods to create_from_escaped_token, create_from_escaped_tokens, and create_from_descriptors clarifies their purpose and improves code readability, making the codebase more maintainable.
components/core/src/clp_s/search/kql/kql.cpp (3)
2-2: Include <stdexcept> for exception handling
Adding the <stdexcept> header allows the use of standard exceptions like std::runtime_error, ensuring proper error handling.
Line range hint 73-77: Fix potential out-of-bounds access by checking for empty string
Adding the check false == text.empty() before accessing text.at(0) prevents potential exceptions when text is empty, enhancing the robustness of the unquote_string function.
129-129: Update method calls to use new creation methods
Using create_from_escaped_tokens and create_from_escaped_token in visitColumn and visitValue_expression ensures proper handling of escaped tokens and aligns with the updated ColumnDescriptor interface.
Also applies to: 205-205
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/core/tests/test-kql.cpp (1)
243-270: LGTM: Comprehensive escape sequence test coverage with suggestionThe new test section thoroughly covers various escape sequences including Unicode, control characters, and special characters. The test structure is consistent with other sections and includes proper validation.
Consider adding these additional test cases:
auto translated_pair = GENERATE( std::pair{"\\\\", "\\\\"}, + std::pair{"\\u0000", "\0"}, // Null character + std::pair{"\\u0001\\u001F", "\1\37"}, // Control characters + std::pair{"香港", "香港"}, // Direct Unicode // ... existing cases ... );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/core/tests/test-kql.cpp(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
components/core/tests/test-kql.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🔇 Additional comments (2)
components/core/tests/test-kql.cpp (2)
6-6: LGTM: fmt library inclusion
The addition of the fmt library is appropriate for safe string formatting in the new test cases.
69-70: LGTM: Consistent use of create_descriptor_from_escaped_token
The replacement of direct DescriptorToken instantiation with create_descriptor_from_escaped_token is consistent throughout the test file and aligns with the PR's objective of improving escape sequence handling.
Also applies to: 87-88, 133-134, 183-184, 214-217
| static std::shared_ptr<ColumnDescriptor> create(std::string const& descriptor); | ||
| static std::shared_ptr<ColumnDescriptor> create(std::vector<std::string> const& descriptors); | ||
| static std::shared_ptr<ColumnDescriptor> create(DescriptorList const& descriptors); | ||
| static std::shared_ptr<ColumnDescriptor> create_from_escaped_token(std::string const& token); |
There was a problem hiding this comment.
Can we modify the description above to reflect the changes?
| if (is_value) { | ||
| unescaped.append("\\?"); | ||
| } else { | ||
| unescaped.push_back('?'); |
There was a problem hiding this comment.
So we add a branch here in case a user escapes ? in the key?
There was a problem hiding this comment.
A little bit different.
Right now we have four different classes of escaping rules: the kql column escaping rules, the kql value escaping rules, the c++ column escaping rules, and the c++ search predicate escaping rules.
When taking a kql query we need to convert kql column escaping -> c++ column escaping and kql value escaping -> c++ search predicate escaping.
This branch in particular deals with a difference between the c++ column escaping rules and the c++ search predicate escaping rules. In particular, the ? character has the special meaning in search of matching one character, but the ? character has no special meaning for columns. That means that the c++ search code expects a literal ? character to be escaped, but the c++ column code has no such expectation.
components/core/src/clp_s/Utils.cpp
Outdated
| for (size_t i = 0; i < value.size(); ++i) { | ||
| if (false == escaped && '\\' != value[i]) { | ||
| unescaped.push_back(value[i]); | ||
| continue; | ||
| } else if (false == escaped && '\\' == value[i]) { | ||
| escaped = true; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Can we do something like
if (false == escaped) {
if ('\\' == value[i]) {
} else {
}
}| void StringUtils::escape_json_string(std::string& destination, std::string_view const source) { | ||
| // Escaping is implemented using this `append_unescaped_slice` approach to offer a fast path | ||
| // when strings are mostly or entirely valid escaped JSON. Benchmarking shows that this offers | ||
| // a net decompression speedup of ~30% compared to adding every character to the destination one |
There was a problem hiding this comment.
The speedup is really impressive.
There was a problem hiding this comment.
Yeah, I was surprised too lol. My hope was that the fast path would save some memory allocations and let the compiler play some simd tricks, and it seems to have worked.
| // Note: we only need to compare the m_token field because m_regex and m_wildcard are | ||
| // derived from m_token. | ||
| return m_token == rhs.m_token; | ||
| return m_token == rhs.m_token && m_wildcard == rhs.m_wildcard; |
There was a problem hiding this comment.
Why do we need to compare m_wildcard here?
There was a problem hiding this comment.
Yes -- the current implementation now receives an escaped token, unescapes it, and sets relevant flags. Since the implementation unescapes the string it stores internally the flags need to be used to differentiate some cases.
In particular if you construct a DescriptorToken with \* it will internally store * and the m_wildcard flag will be false, whereas if you construct it with * it will again store * but the m_wildcard flag will be true.
| @@ -69,7 +70,7 @@ class ParseTreeVisitor : public KqlBaseVisitor { | |||
|
|
|||
| public: | |||
| static std::string unquote_string(std::string const& text) { | |||
| if (text.at(0) == '"') { | |||
| if (false == text.empty() && '"' == text.at(0)) { | |||
There was a problem hiding this comment.
Do we need to do this empty check?
There was a problem hiding this comment.
Technically no because of the way our kql grammar is written and the way this is called, but I figured we should probably handle this edge case in case we ever write an incorrect caller or change the grammar.
Co-authored-by: wraymo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/core/src/clp_s/Utils.cpp (3)
436-485: Consider simplifying the escaped character handling logicThe implementation is correct and handles escaping properly. However, the readability could be improved by consolidating the escape handling conditions.
Consider this alternative structure:
- if (false == escaped) { - if ('\\' == descriptor[i]) { - escaped = true; - } else if ('.' == descriptor[i]) { + if ('\\' == descriptor[i] && !escaped) { + escaped = true; + continue; + } + + if (!escaped) { + if ('.' == descriptor[i]) {
546-568: Consider pre-allocating the bufferThe implementation is secure and correct. However, there's an opportunity for a minor optimization.
Consider pre-allocating the buffer to avoid reallocation:
- std::string buf = "\"\\u"; + std::string buf; + buf.reserve(8 + simdjson::SIMDJSON_PADDING); // "\\uXXXX" + padding + buf = "\"\\u";
575-674: Consider using early continue for the main pathThe implementation is correct and handles all cases properly. However, the readability could be improved by restructuring the main path.
Consider this alternative structure for the main loop:
- if (false == escaped && '\\' != value[i]) { - unescaped.push_back(value[i]); - continue; - } else if (false == escaped && '\\' == value[i]) { + if (!escaped) { + if ('\\' == value[i]) { + escaped = true; + continue; + } + unescaped.push_back(value[i]); + continue; + }components/core/src/clp_s/search/ColumnDescriptor.hpp (1)
65-99: Consider enhancing error handling and documentationWhile the implementation is solid, consider these improvements:
- Document the behaviour for empty tokens
- Add validation for maximum token length
- Consider using a more descriptive error message for the trailing backslash case
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/core/src/clp_s/Utils.cpp(2 hunks)components/core/src/clp_s/Utils.hpp(3 hunks)components/core/src/clp_s/search/ColumnDescriptor.hpp(4 hunks)components/core/src/clp_s/search/kql/Kql.g4(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
components/core/src/clp_s/Utils.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/Utils.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/search/ColumnDescriptor.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🔇 Additional comments (14)
components/core/src/clp_s/Utils.hpp (6)
213-214: LGTM! Documentation clearly describes the function's purpose.
The updated documentation effectively communicates that this method handles KQL string column descriptors and their escaping rules.
222-236: LGTM! Well-documented JSON string escaping implementation.
The documentation thoroughly explains the JSON spec requirements and handling of control sequences. The method signature appropriately uses string_view for the source parameter, promoting efficient string handling.
238-250: LGTM! Clear documentation of KQL value unescaping behaviour.
The method appropriately preserves essential escape sequences ('\', '*', '?') while unescaping others, which is crucial for correct CLP search interpretation.
277-288: LGTM! Efficient hex conversion implementation.
The implementation correctly handles both decimal and hexadecimal nibbles, using the fixed subtraction value as noted in past reviews.
296-300: LGTM! Efficient Unicode escape sequence generation.
The method efficiently reuses char_to_hex and minimizes string allocations by using append.
308-308: Fix typo in documentation.
The word "successfully" is misspelled.
- * @return true if the value was unescaped succesfully and false otherwise.
+ * @return true if the value was unescaped successfully and false otherwise.components/core/src/clp_s/Utils.cpp (2)
4-4: LGTM: simdjson inclusion is appropriate
The inclusion of simdjson.h is well-placed and necessary for efficient JSON parsing operations.
487-539: Excellent performance optimization for JSON string escaping
The implementation using append_unescaped_slice provides a significant performance improvement, especially for strings that require minimal escaping. The approach is well-documented and handles all necessary escape sequences correctly.
components/core/src/clp_s/search/kql/Kql.g4 (2)
61-61: LGTM: Addition of UNICODE support to unquoted characters
The addition of UNICODE to the UNQUOTED_CHARACTER fragment properly enables support for \u unicode escape sequences in unquoted literals, aligning with the PR objectives.
95-96: LGTM: Well-defined UNICODE escape sequence format
The UNICODE and HEXDIGIT fragments are correctly defined to ensure that unicode escape sequences match exactly 4 hexadecimal digits, following the standard format \uXXXX.
components/core/src/clp_s/search/ColumnDescriptor.hpp (4)
19-24: LGTM: Well-structured exception class
The OperationFailed exception class properly extends TraceableException and maintains consistent error handling patterns.
27-42: LGTM: Well-designed factory methods
The static factory methods provide a clear distinction between escaped and literal token creation, improving API usability and encapsulating token processing logic.
124-130: LGTM: Clear and consistent factory methods
The static factory methods for ColumnDescriptor provide a clear API that aligns well with the new token handling approach.
138-144:
Add iterator validation
The insert method should validate that the provided iterator belongs to m_descriptors to prevent undefined behaviour.
Add validation at the beginning of the method:
void insert(DescriptorList::iterator pos, DescriptorList const& source) {
+ if (pos < m_descriptors.begin() || pos > m_descriptors.end()) {
+ throw OperationFailed(ErrorCodeBadParam, __FILENAME__, __LINE__);
+ }
m_descriptors.insert(pos, source.begin(), source.end());
check_and_set_unresolved_descriptor_flag();
if (is_unresolved_descriptor()) {
simplify_descriptor_wildcards();
}
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
components/core/src/clp_s/search/ColumnDescriptor.cpp (1)
Line range hint
25-41: Consider using initialization lists for better performanceThe constructors could benefit from using initialization lists instead of in-body initialization.
Here's the suggested improvement:
-ColumnDescriptor::ColumnDescriptor(std::string const& token) { - m_flags = cAllTypes; - m_descriptors.emplace_back(DescriptorToken::create_descriptor_from_escaped_token(token)); +ColumnDescriptor::ColumnDescriptor(std::string const& token) + : m_flags(cAllTypes) + , m_descriptors{DescriptorToken::create_descriptor_from_escaped_token(token)} { -ColumnDescriptor::ColumnDescriptor(std::vector<std::string> const& tokens) { - m_flags = cAllTypes; - m_descriptors = std::move(tokenize_descriptor(tokens)); +ColumnDescriptor::ColumnDescriptor(std::vector<std::string> const& tokens) + : m_flags(cAllTypes) + , m_descriptors(tokenize_descriptor(tokens)) {🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 36-36: Variable 'm_descriptors' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
🧹 Nitpick comments (6)
components/core/src/clp_s/search/ColumnDescriptor.cpp (1)
52-67: Consider using std::make_shared for better exception safetyWhile the factory methods work correctly, using
std::make_sharedwould provide better exception safety and potentially better performance.Here's the suggested improvement:
-std::shared_ptr<ColumnDescriptor> ColumnDescriptor::create_from_escaped_token( - std::string const& token -) { - return std::shared_ptr<ColumnDescriptor>(new ColumnDescriptor(token)); +std::shared_ptr<ColumnDescriptor> ColumnDescriptor::create_from_escaped_token( + std::string const& token +) { + return std::make_shared<ColumnDescriptor>(token);Apply similar changes to the other factory methods.
components/core/src/clp_s/Utils.cpp (3)
436-485: Consider restructuring for better readabilityThe function could be more readable with early returns and clearer separation of concerns.
Consider this refactor:
bool StringUtils::tokenize_column_descriptor( std::string const& descriptor, std::vector<std::string>& tokens ) { std::string cur_tok; bool escaped{false}; for (size_t i = 0; i < descriptor.size(); ++i) { if (false == escaped) { if ('\\' == descriptor[i]) { escaped = true; continue; } if ('.' == descriptor[i]) { if (cur_tok.empty()) { return false; } std::string unescaped_token; if (false == unescape_kql_internal(cur_tok, unescaped_token, false)) { return false; } tokens.push_back(unescaped_token); cur_tok.clear(); continue; } cur_tok.push_back(descriptor[i]); continue; } escaped = false; if ('.' == descriptor[i]) { cur_tok.push_back('.'); continue; } cur_tok.push_back('\\'); cur_tok.push_back(descriptor[i]); } if (escaped || cur_tok.empty()) { return false; } std::string unescaped_token; if (false == unescape_kql_internal(cur_tok, unescaped_token, false)) { return false; } tokens.push_back(unescaped_token); return true; }
546-568: Consider enhancing error handling and buffer managementWhile the implementation is solid, there are a few potential improvements:
- The error handling could be more specific about what went wrong.
- The buffer size could be pre-calculated more precisely.
Consider this enhancement:
bool convert_four_byte_hex_to_utf8(std::string_view const hex, std::string& destination) { std::string buf = "\"\\u"; + buf.reserve(8 + simdjson::SIMDJSON_PADDING); // "\\uXXXX" + padding buf += hex; buf.push_back('"'); - buf.reserve(buf.size() + simdjson::SIMDJSON_PADDING); simdjson::ondemand::parser parser; auto value = parser.iterate(buf); try { if (false == value.is_scalar()) { - return false; + SPDLOG_DEBUG("Expected scalar value for hex sequence: {}", hex); + return false; } if (simdjson::ondemand::json_type::string != value.type()) { + SPDLOG_DEBUG("Expected string value for hex sequence: {}", hex); return false; } std::string_view unescaped_utf8 = value.get_string(false); destination.append(unescaped_utf8); } catch (std::exception const& e) { + SPDLOG_DEBUG("Failed to parse hex sequence: {} - {}", hex, e.what()); return false; } return true; }
575-675: Consider using constants for special charactersThe implementation is thorough, but readability could be improved by using named constants for special characters and escape sequences.
Consider this enhancement:
+namespace { + constexpr char kEscapeChar = '\\'; + constexpr char kWildcardStar = '*'; + constexpr char kWildcardQuestion = '?'; + const std::string kEscapedBackslash = "\\\\"; +} // namespace bool StringUtils::unescape_kql_internal( std::string const& value, std::string& unescaped, bool is_value ) { bool escaped{false}; for (size_t i = 0; i < value.size(); ++i) { if (false == escaped) { - if ('\\' == value[i]) { + if (kEscapeChar == value[i]) { escaped = true; } else { unescaped.push_back(value[i]); } continue; }components/core/src/clp_s/search/ColumnDescriptor.hpp (2)
36-42: Documentation could be more detailedConsider enhancing the documentation for
create_descriptor_from_literal_tokenby:
- Adding
@param tokendescription- Adding
@returndescription- Including examples of usage
81-94: Consider optimizing escape sequence processingThe current implementation could be more efficient by:
- Pre-allocating the result string based on token size
- Using string_view's substr capabilities instead of character-by-character processing
- bool escaped{false}; - for (size_t i = 0; i < token.size(); ++i) { - if (false == escaped) { - if ('\\' == token[i]) { - escaped = true; - } else { - m_token.push_back(token[i]); - } - continue; - } else { - m_token.push_back(token[i]); - escaped = false; - } - } + m_token.reserve(token.size()); + size_t pos = 0; + while (pos < token.size()) { + if (token[pos] == '\\' && pos + 1 < token.size()) { + m_token.push_back(token[pos + 1]); + pos += 2; + } else { + m_token.push_back(token[pos++]); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/core/src/clp_s/Utils.cpp(2 hunks)components/core/src/clp_s/search/ColumnDescriptor.cpp(3 hunks)components/core/src/clp_s/search/ColumnDescriptor.hpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
components/core/src/clp_s/search/ColumnDescriptor.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/search/ColumnDescriptor.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/Utils.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🪛 cppcheck (2.10-2)
components/core/src/clp_s/search/ColumnDescriptor.cpp
[performance] 36-36: Variable 'm_descriptors' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
🔇 Additional comments (6)
components/core/src/clp_s/search/ColumnDescriptor.cpp (2)
18-22: LGTM! Clear and focused logic
The simplified condition for setting the unresolved descriptor flag improves code clarity by focusing solely on wildcard tokens.
9-9: LGTM! Verify token creation consistency
The change to use create_descriptor_from_escaped_token aligns well with the PR's objective of improving escape sequence handling.
Let's verify consistent usage across the codebase:
✅ Verification successful
Token creation implementation is consistent across the codebase
The verification shows that DescriptorToken creation is consistently implemented using the static factory methods:
create_descriptor_from_escaped_tokenis used inColumnDescriptor.cppand test casescreate_descriptor_from_literal_tokenis used inSchemaMatch.cpp- No direct instantiation of
DescriptorTokenwas found
The implementation aligns with the PR objectives and maintains consistent token handling throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct instantiation of DescriptorToken that might have been missed
ast-grep --pattern 'DescriptorToken($$$)'
Length of output: 43
Script:
#!/bin/bash
# Search for DescriptorToken class usage and instantiation patterns
rg -A 2 "DescriptorToken::" --type cpp
# Also search for create_descriptor patterns
rg -A 2 "create_descriptor" --type cpp
Length of output: 5972
components/core/src/clp_s/Utils.cpp (1)
487-539: Excellent implementation with proven performance benefits!
The implementation is well-optimized using a slice-based approach that provides a fast path for mostly valid JSON strings. The 30% decompression speedup mentioned in the comments is impressive.
components/core/src/clp_s/search/ColumnDescriptor.hpp (3)
19-24: LGTM: Well-structured exception class
The OperationFailed exception class is properly implemented with appropriate inheritance and constructor parameters.
61-61: LGTM: Proper equality comparison
The equality operator correctly compares both the token string and wildcard status, ensuring proper token matching behaviour.
151-157: Consider adding iterator validation
The insert method should validate that:
- The iterator
posis valid and belongs tom_descriptors - The insertion won't exceed any size limits
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/core/src/clp_s/search/Output.cpp (1)
669-669: Consider performance optimization for repeated key comparisons.As mentioned in the PR objectives, there's a noted moderate decrease in decompression performance (1.19x increase in time). Since this method is called for each field in the object, consider caching the unescaped key if the same field is accessed multiple times.
Consider this optimization:
- if (field.unescaped_key(true).value() != unresolved_tokens[cur_idx].get_token()) { + auto unescaped = field.unescaped_key(true).value(); + if (unescaped != unresolved_tokens[cur_idx].get_token()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/core/src/clp_s/search/Output.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
components/core/src/clp_s/search/Output.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🔇 Additional comments (1)
components/core/src/clp_s/search/Output.cpp (1)
669-669: LGTM: Using unescaped key comparison aligns with the new archive format.
The change from field.key() to field.unescaped_key(true).value() correctly implements the requirement to handle unescaped string values during search operations.
Let's verify the impact on search functionality:
✅ Verification successful
The change to use unescaped_key() is consistent with existing codebase patterns
The search results confirm that unescaped_key(true) is already being used consistently in other parts of the codebase, particularly in JsonParser.cpp. The change in Output.cpp aligns with this established pattern for key comparisons. The get_token() usage in comparison operations is also consistent across the codebase, appearing in similar comparison contexts in EvaluateTimestampIndex.cpp and ColumnDescriptor.cpp.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other key comparisons in the codebase that might need similar updates
# Search for key() usage in array/object handling code
rg -A 5 'field\.key\(\)' --type cpp
# Search for similar patterns that might need updating
ast-grep --pattern 'field.key()'
Length of output: 72
Script:
#!/bin/bash
# Let's try a broader search for key-related comparisons and field access patterns
# Search for key comparisons in the codebase
rg -A 5 '\.key\(\)' --type cpp
# Search for unescaped_key usage
rg -A 5 '\.unescaped_key\(' --type cpp
# Search for get_token comparisons
rg -A 5 '\.get_token\(\)' --type cpp
Length of output: 3062
Description
This PR changes the archive format of clp-s to store the unescaped versions of string values instead of the escaped version of string values, and implements fixes and missing features for search involving escape sequences.
This change to the archive format both makes it more straightforward to support correct search on escape sequences and makes it so that the archive format is consistent between the JSON ingestion path and the soon-to-be-implemented kv-pair IR ingestion path.
Compression ratio is effectively unchanged (1.002x improvement in compression ratio on average.
Overall there is effectively no change in compression performance, and only a moderate decrease in decompression performance. This performance is achieved by getting rid of string copies during compression and implementing escape for JSON strings with a fast path for strings that largely don't have to be escaped.
On the search side the KQL escaping behaviour has been nailed down, support for the \KEYWORD (e.g. \and) escape sequences has been removed, and support for the \u unicode escape sequences has been added.
The full set of escape sequences are:
Note that the LITERAL escape sequences are for getting the literal version of a character as opposed to a special character that affects the semantics of search (wildcard "*", match any single character "?", and denote nested keys keys ".").
Validation performed
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor