feat: Add support for CLP JSON string serialization.#40
Conversation
WalkthroughAdds lazy support for the Changes
Sequence Diagram(s)sequenceDiagram
participant Exec as Query Executor
participant Cursor as CLP Cursor (Archive / IR)
participant Loader as Clp*JsonStringVectorLoader
participant Source as SchemaReader / IR source
Exec->>Cursor: loadSplit(projection)
activate Cursor
Cursor->>Cursor: parse projection, detect "__json_string"
Cursor->>Cursor: record jsonStringColumnIndices_, set columnIndex_/projectedColumnIndex_
deactivate Cursor
Exec->>Cursor: createVectorHelper(column)
activate Cursor
Cursor->>Cursor: is column in jsonStringColumnIndices_?
alt json_string column
Cursor->>Loader: construct Loader(SourcePtr, filteredRows)
activate Loader
Loader-->>Cursor: return LazyVector(loader)
deactivate Loader
else regular column
Cursor->>Source: materialize column data
Source-->>Cursor: populated Vector
end
deactivate Cursor
Note over Loader,Source: When LazyVector accessed, Loader serializes per-row to JSON
Loader->>Source: serializeRowToJson(rowIndex)
Source-->>Loader: json_string
Loader->>Loader: write into StringVector
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wraymo
changed the title
wraymo
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: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
CMake/resolve_dependency_modules/clp.cmake(1 hunks)velox/connectors/clp/search_lib/archive/CMakeLists.txt(1 hunks)velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp(7 hunks)velox/connectors/clp/search_lib/archive/ClpArchiveCursor.h(2 hunks)velox/connectors/clp/search_lib/archive/ClpArchiveJsonStringVectorLoader.cpp(1 hunks)velox/connectors/clp/search_lib/archive/ClpArchiveJsonStringVectorLoader.h(1 hunks)velox/connectors/clp/tests/ClpConnectorTest.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T18:00:51.241Z
Learnt from: anlowee
PR: y-scope/velox#25
File: velox/connectors/clp/search_lib/archive/ClpArchiveCursor.h:36-39
Timestamp: 2025-08-20T18:00:51.241Z
Learning: In ClpArchiveCursor.h, the clp_s::InputSource type is available through transitive inclusion via BaseClpCursor.h, which includes "clp_s/InputConfig.hpp" where InputSource is defined. The constructor parameter type is properly accessible without needing direct inclusion.
Applied to files:
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.h
🧬 Code graph analysis (5)
velox/connectors/clp/search_lib/archive/ClpArchiveJsonStringVectorLoader.h (2)
velox/connectors/clp/search_lib/archive/ClpArchiveJsonStringVectorLoader.cpp (3)
ClpArchiveJsonStringVectorLoader(25-30)loadInternal(32-47)loadInternal(32-36)velox/vector/LazyVector.h (1)
VectorLoader(177-210)
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.h (1)
velox/connectors/clp/search_lib/archive/ClpQueryRunner.h (5)
clp_s(21-26)clp_s(28-31)clp_s(33-35)vector(76-78)ClpQueryRunner(43-88)
velox/connectors/clp/tests/ClpConnectorTest.cpp (1)
velox/python/type/type.pyi (1)
VARCHAR(29-29)
velox/connectors/clp/search_lib/archive/ClpArchiveJsonStringVectorLoader.cpp (1)
velox/connectors/clp/search_lib/archive/ClpArchiveJsonStringVectorLoader.h (1)
ClpArchiveJsonStringVectorLoader(29-44)
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp (2)
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.h (2)
projectedColumnIndex_(69-69)columnIndex_(68-68)velox/connectors/clp/search_lib/archive/ClpQueryRunner.h (1)
vector(76-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (18)
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.h (3)
19-19: LGTM!The
<unordered_set>include is necessary for thejsonStringColumnIndices_member.
25-25: LGTM!Forward declaration is appropriate. Based on learnings, the type is available through transitive inclusion when needed.
60-70: LGTM!The new members properly support the per-column indexing scheme for JSON string handling. The renaming from
readerIndex_tocolumnIndex_improves clarity.velox/connectors/clp/search_lib/archive/CMakeLists.txt (1)
18-18: LGTM!The new JSON string vector loader source is correctly added to the build.
velox/connectors/clp/tests/ClpConnectorTest.cpp (2)
210-261: LGTM!The test comprehensively validates JSON string serialization with proper assertions for the
__json_stringcolumn alongside regular fields.
441-475: LGTM!The test properly validates JSON string serialization with KQL pushdown filters and verifies the complete JSON payload structure.
velox/connectors/clp/search_lib/archive/ClpArchiveJsonStringVectorLoader.cpp (2)
25-30: LGTM!The constructor properly initializes members and sets up the serializer.
41-46: Since clp_s is an external library, let me search for documentation about it and the return type ofgenerate_json_string:Let me search specifically for the clp_s library source code and the SchemaReader class:
Based on my investigation, I was unable to locate the exact source code for
clp_s::SchemaReader::generate_json_stringto verify the return type and lifetime guarantees. While I confirmed that clp_s is part of YScope's open-source Compressed Log Processor library, the specific method definition is not accessible in the web search results or the repository files I checked.The original review comment raises a legitimate concern about StringView lifetime safety that requires verification from someone with access to the clp_s library documentation or source code.
StringView lifetime safety must be verified.
Line 44 constructs a
StringViewfrom the return value ofschemaReader_->generate_json_string(messageIndex). Confirm that this method returns a string with appropriate lifetime (managed buffer inschemaReader_) rather than a temporary or reference, to avoid dangling pointers whenStringViewis stored in the vector.velox/connectors/clp/search_lib/archive/ClpArchiveJsonStringVectorLoader.h (1)
31-43: LGTM!The class interface is well-designed with appropriate encapsulation. The constructor signature matches the implementation, and the
loadInternaloverride follows theVectorLoadercontract.velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp (8)
24-24: LGTM!Including the new JSON string vector loader header is necessary for the lazy loading implementation below.
49-50: LGTM!Resetting both column indices at the start of
fetchNextensures proper state for the next batch.
74-76: LGTM!The refactored approach using
schemaReader_andinitialize_filter_with_column_mapproperly integrates the schema reader for JSON string generation.
104-104: LGTM!The validation correctly accounts for JSON string columns that are handled separately from the projected columns.
154-159: LGTM!The detection of
__json_stringcolumns and recording their indices is clean and correctly skips further processing for these special columns.
247-256: LGTM!Pre-sizing the children vector with
reserveand using explicitnumChildrenimproves efficiency by avoiding reallocations during vector construction.
261-271: LGTM!The lazy loading path for JSON string columns is well-implemented, using the new
ClpArchiveJsonStringVectorLoaderto generate JSON strings on demand.
273-286: LGTM!The updated column access pattern correctly uses
projectedColumnIndex_to index intoprojectedColumnsand retrieves the corresponding type fromoutputColumns_.CMake/resolve_dependency_modules/clp.cmake (1)
19-19: Commit 9e991ab49681faff0aea259a37b489c3f416e06e exists and is accessible in the CLP repository.Verification confirms the commit is available and publicly accessible via both direct git fetch and GitHub API. The commit has been successfully integrated into the y-scope/clp repository.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
velox/connectors/clp/search_lib/archive/ClpArchiveJsonStringVectorLoader.h(1 hunks)velox/connectors/clp/tests/ClpConnectorTest.cpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
velox/connectors/clp/tests/ClpConnectorTest.cpp (1)
velox/python/type/type.pyi (1)
VARCHAR(29-29)
velox/connectors/clp/search_lib/archive/ClpArchiveJsonStringVectorLoader.h (2)
velox/connectors/clp/search_lib/archive/ClpArchiveJsonStringVectorLoader.cpp (3)
ClpArchiveJsonStringVectorLoader(25-30)loadInternal(32-47)loadInternal(32-36)velox/vector/LazyVector.h (1)
VectorLoader(177-210)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (4)
velox/connectors/clp/search_lib/archive/ClpArchiveJsonStringVectorLoader.h (2)
23-42: Clean class design for on-demand JSON string loading.The
ClpArchiveJsonStringVectorLoaderfollows the VectorLoader pattern correctly and provides a focused API for generating serialized JSON strings from CLP archives. The design aligns well with Velox's lazy loading architecture and the PR's objective of supporting__json_stringcolumns.
29-35: Verify lifecycle management of the raw SchemaReader pointer.The class stores a raw pointer to
SchemaReader*retrieved fromarchiveReader_->read_schema_table()(ClpArchiveCursor.cpp line 74–75). This pointer is passed to the loader which wraps it in aLazyVectorfor deferred access. The concern is valid: ifschemaReader_is reassigned in a subsequent loop iteration before theLazyVectoraccesses the loader, the raw pointer becomes stale. Ensure that:
- The reference lifetime from
read_schema_table()extends through the entire vector materialization cycle, or- Add documentation confirming that
createVector()is always called synchronously within the same batch beforeschemaReader_can be reassigned, or- Consider storing the
SchemaReaderreference directly or using a shared ownership model to guarantee validity across deferred access.velox/connectors/clp/tests/ClpConnectorTest.cpp (2)
210-261: Solid test coverage for JSON string projection with filtering.The test correctly verifies that
__json_stringcolumns return fully serialized JSON payloads for filtered rows. The expected JSON strings accurately reflect the complete row data, including null handling (line 250 shows"userId":null), and the test structure follows the established patterns in this file.
440-475: Review comment is incorrect — JSON string support is Archive-specific, not intended for IR splits.The code evidence shows JSON string loading is implemented exclusively in
ClpArchiveCursor(viajsonStringColumnIndices_member variable) with no corresponding functionality inClpIrCursor. Bothtest1JsonStringandtest2JsonStringuse onlySplitType::kArchive. This architectural separation indicates JSON strings are an Archive-only feature rather than a gap requiring IR support. No action is needed.Likely an incorrect or invalid review comment.
wraymo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp (2)
217-237: Incorrect column mapping when JSON-string columns are present.projectedColumnIndex_ indexes the list of non-JSON projected columns, but you read outputColumns_[projectedColumnIndex_], which includes JSON columns. This misaligns names/types once a __json_string appears before a non-JSON column (e.g., [requestId, __json_string, method]). Result: method is paired with "__json_string" metadata and wrong node-ids.
Apply a mapping from projected index → original output index (skip JSON columns):
- auto projectedColumn = outputColumns_[projectedColumnIndex_]; - auto projectedColumnType = projectedColumn.type; - auto it = projectedColumnIdxNodeIdsMap_.find(projectedColumnIndex_); + // Map projected (non-JSON) index to the corresponding index in outputColumns_. + auto toOutputIndex = [&](size_t projectedIdx) -> size_t { + size_t count = 0; + for (size_t i = 0; i < outputColumns_.size(); ++i) { + if (jsonStringColumnIndices_.find(i) != jsonStringColumnIndices_.end()) { + continue; + } + if (count == projectedIdx) { + return i; + } + ++count; + } + VELOX_FAIL("Projected column index {} out of range ({} non-JSON columns).", + projectedIdx, count); + }; + const size_t outIdx = toOutputIndex(projectedColumnIndex_); + auto projectedColumn = outputColumns_[outIdx]; + auto projectedColumnType = projectedColumn.type; + auto it = projectedColumnIdxNodeIdsMap_.find(projectedColumnIndex_); @@ - projectedColumnIndex_++; + ++projectedColumnIndex_;Please add a unit/integration test with a projection like [__json_string, method] and another like [requestId, __json_string, method] to catch this class of bugs.
59-70: Optional: reset indices in createVector for robustness.Resetting columnIndex_/projectedColumnIndex_ in fetchNext works today but is fragile if createVector is called more than once per fetch. Consider resetting them at the start of createVector.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
velox/connectors/clp/search_lib/ir/CMakeLists.txt(1 hunks)velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp(4 hunks)velox/connectors/clp/search_lib/ir/ClpIrCursor.h(1 hunks)velox/connectors/clp/search_lib/ir/ClpIrJsonStringVectorLoader.cpp(1 hunks)velox/connectors/clp/search_lib/ir/ClpIrJsonStringVectorLoader.h(1 hunks)velox/connectors/clp/tests/ClpConnectorTest.cpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp (1)
velox/connectors/clp/search_lib/ir/ClpIrCursor.h (2)
columnIndex_(89-89)projectedColumnIndex_(90-90)
velox/connectors/clp/search_lib/ir/ClpIrJsonStringVectorLoader.h (2)
velox/vector/LazyVector.h (1)
VectorLoader(177-210)velox/connectors/clp/search_lib/ir/ClpIrJsonStringVectorLoader.cpp (2)
loadInternal(24-48)loadInternal(24-28)
velox/connectors/clp/search_lib/ir/ClpIrCursor.h (1)
velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp (2)
splitFieldsToNamesAndTypes(124-163)splitFieldsToNamesAndTypes(125-125)
🪛 GitHub Actions: Run Checks
velox/connectors/clp/search_lib/ir/ClpIrJsonStringVectorLoader.h
[error] 1-1: End-of-file-fixer hook failed. The file was modified to add a newline at end of file.
velox/connectors/clp/search_lib/ir/ClpIrJsonStringVectorLoader.cpp
[error] 1-1: End-of-file-fixer hook failed. The file was modified to add a newline at end of file.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (3)
velox/connectors/clp/search_lib/ir/CMakeLists.txt (1)
18-19: LGTM — source list updated correctly.ClpIrJsonStringVectorLoader.cpp is included; linking remains appropriate. If this header is intended to be public/installed, ensure install rules export it.
velox/connectors/clp/search_lib/ir/ClpIrJsonStringVectorLoader.cpp (1)
1-50: Commit the updated file to fix the pre-commit EOF newline issue.The end-of-file-fixer hook ensures that a file either is empty or ends with one newline. The CI hook detected that this file lacked a trailing newline and automatically added one. Commit the modified file to satisfy the pre-commit hook.
velox/connectors/clp/search_lib/ir/ClpIrJsonStringVectorLoader.h (1)
19-22: Remove unused BaseClpCursor.h include from both files.The
BaseClpCursor.hinclude is not used in either the header or implementation file. The class inherits fromVectorLoader(notBaseClpCursor), and the implementation makes no reference to any types or symbols from BaseClpCursor. Remove the include from both line 20 in the header and line 19 in the implementation file.⛔ Skipped due to learnings
Learnt from: anlowee PR: y-scope/velox#25 File: velox/connectors/clp/search_lib/ClpVectorLoader.cpp:24-24 Timestamp: 2025-08-20T18:13:32.442Z Learning: In ClpVectorLoader.cpp, the include of BaseClpCursor.h is necessary because the implementation uses ColumnType enum values (like ColumnType::Integer, ColumnType::Float) in a switch statement. While ClpVectorLoader.h has a forward declaration of ColumnType, the full definition from BaseClpCursor.h is required in the implementation file to access the enum values.
| void loadInternal( | ||
| RowSet rows, | ||
| ValueHook* hook, | ||
| vector_size_t resultSize, | ||
| VectorPtr* result) override; |
There was a problem hiding this comment.
Critical: Use-after-free bug in the implementation.
While reviewing the header, I noticed a critical bug in the implementation file ClpIrJsonStringVectorLoader.cpp (lines 40-41):
std::string const jsonString = serializedResult.value().second.dump();
stringVector->set(vectorIndex, StringView(jsonString));The jsonString variable is local to the loop iteration and will be destroyed, causing the StringView to point to deallocated memory. The string data must be copied into the vector's buffer.
The typical pattern in Velox is to use the vector's buffer or ensure the StringView points to persistent storage. Please verify that FlatVector<StringView>::set correctly handles copying the data, or modify the code to explicitly manage the string lifetime, for example:
std::string const jsonString = serializedResult.value().second.dump();
stringVector->set(vectorIndex, StringView(jsonString.data(), jsonString.size()));Additionally, verify this pattern against similar VectorLoader implementations in the codebase to ensure correctness.
🤖 Prompt for AI Agents
In velox/connectors/clp/search_lib/ir/ClpIrJsonStringVectorLoader.cpp around
lines 40-41, the code creates a temporary std::string and passes it to
StringView which results in a use-after-free when the temporary is destroyed;
change the code to ensure the vector owns a copy of the bytes: create the JSON
string, then pass StringView(jsonString.data(), jsonString.size()) to
stringVector->set so the vector can copy the bytes (or, if
FlatVector<StringView>::set does not copy, explicitly copy the bytes into the
vector's owned buffer before setting the element). After making the change,
verify the pattern against other VectorLoader implementations in the codebase to
match Velox conventions.
There was a problem hiding this comment.
When setting the vector, the string is actually copied
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the FlatVector::set implementation for StringView
echo "=== Searching for FlatVector::set implementation ==="
ast-grep --pattern 'void set($_, StringView $_) {
$$$
}'
echo -e "\n=== Searching for FlatVector set method definitions ==="
rg -nP --type=cpp -A 10 'void\s+set\s*\(\s*vector_size_t.*StringView' velox/vector/
echo -e "\n=== Looking for StringView handling in FlatVector ==="
rg -nP --type=cpp -A 15 'class FlatVector.*StringView' velox/vector/
echo -e "\n=== Examining similar patterns in other VectorLoader implementations ==="
rg -nP --type=cpp -B 3 -A 3 'stringVector->set.*StringView' velox/
Length of output: 6332
🏁 Script executed:
#!/bin/bash
# Find FlatVector header files
echo "=== Finding FlatVector header files ==="
fd -e h -e hpp FlatVector velox/vector/
echo -e "\n=== Examining FlatVector.h for set method ==="
cat velox/vector/FlatVector.h | grep -A 30 "void set("
echo -e "\n=== Looking for StringView set implementation in FlatVector ==="
rg -nP --type=cpp -A 20 'template.*StringView.*\n.*void set\(' velox/vector/
echo -e "\n=== Searching for setNoCopy in FlatVector ==="
rg -nP --type=cpp -B 5 -A 10 'setNoCopy|setStringViewBufferHolder' velox/vector/FlatVector
Length of output: 1106
🏁 Script executed:
#!/bin/bash
# Look for FlatVector-inl.h which likely has template specializations
echo "=== Examining FlatVector-inl.h for StringView specialization ==="
cat velox/vector/FlatVector-inl.h | head -200
echo -e "\n=== Search for StringView specific set methods ==="
rg -nP --type=cpp -B 3 -A 15 'template<>\s*inline\s*void\s+FlatVector<StringView>::set' velox/vector/
echo -e "\n=== Search for StringView assignment or copy behavior ==="
rg -nP --type=cpp -B 3 -A 10 'StringView.*operator=|StringView.*copy' velox/vector/FlatVector-inl.h
echo -e "\n=== Look at the actual FlatVector-inl.h content for StringView ==="
grep -n -A 30 "template<>" velox/vector/FlatVector-inl.h | grep -A 30 StringView | head -50
Length of output: 7464
🏁 Script executed:
#!/bin/bash
# Search for StringView template specializations or overloads
echo "=== Looking for StringView specific template specializations ==="
rg -nP --type=cpp -B 5 -A 20 'template.*<>\s*.*FlatVector.*StringView|FlatVector<StringView>' velox/vector/ | head -100
echo -e "\n=== Search for setNoCopy specialization for StringView ==="
rg -nP --type=cpp -B 3 -A 20 'setNoCopy.*StringView' velox/vector/
echo -e "\n=== Check how StringView assignment works in the values buffer ==="
rg -nP --type=cpp -B 5 -A 10 'rawValues_\[.*\]\s*=.*StringView|StringView.*rawValues_' velox/vector/FlatVector-inl.h
echo -e "\n=== Look for Buffer or string copying mechanism ==="
rg -nP --type=cpp -B 3 -A 10 'StringView.*Buffer|addStringBuffer|stringBuffers_' velox/vector/FlatVector-inl.h | head -80
Length of output: 13748
🏁 Script executed:
#!/bin/bash
# Look for the StringView specialization of set method
echo "=== Search for template specialization of set for StringView ==="
cat velox/vector/FlatVector-inl.h | grep -B 10 -A 40 "template<>" | grep -B 10 -A 40 "StringView"
echo -e "\n=== Examine the entire setNoCopy implementation section ==="
rg -nP --type=cpp -B 10 -A 30 'template<>\s*inline\s*void\s+FlatVector<StringView>::setNoCopy' velox/vector/
echo -e "\n=== Look for any copying behavior related to StringView in set ==="
rg -nP --type=cpp -C 15 'StringView.*set\(|void set.*StringView' velox/vector/FlatVector-inl.h | head -150
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Look at the complete FlatVector.h to understand set method
echo "=== Looking at the complete generic set method in FlatVector.h ==="
cat velox/vector/FlatVector.h | grep -B 5 -A 15 "void set(vector_size_t idx, T value)"
echo -e "\n=== Check if there's operator= overload for StringView that does copying ==="
rg -nP --type=cpp -B 5 -A 10 'operator=.*StringView|StringView.*operator=' velox/type/
echo -e "\n=== Look for ensureValues or string buffer management ==="
rg -nP --type=cpp -B 3 -A 15 'void ensureValues\(\)' velox/vector/FlatVector-inl.h
echo -e "\n=== Check StringView assignment in the actual code ==="
cat velox/vector/FlatVector-inl.h | grep -B 20 -A 20 "rawValues_\[.*\] = .*value"
Length of output: 5670
🏁 Script executed:
#!/bin/bash
# Look at StringView's inline string capability
echo "=== Examining StringView inline string behavior ==="
rg -nP --type=cpp -B 5 -A 30 'class StringView|struct StringView' velox/type/StringView.h | head -100
echo -e "\n=== Look for how FlatVector handles StringView assignment ==="
rg -nP --type=cpp -B 5 -A 25 'operator=.*inline|setInline|kPrefixSize|kInlineSize' velox/type/StringView.h | head -80
echo -e "\n=== Check if there's automatic buffer management in FlatVector for StringView ==="
rg -nP --type=cpp -B 3 -A 20 'addStringBuffer|acquireSharedStringBuffers|stringBuffer' velox/vector/FlatVector-inl.h | head -100
Length of output: 6534
🏁 Script executed:
#!/bin/bash
# Check the actual StringView size and how large strings are handled
echo "=== Check StringView inline size threshold ==="
grep -n "kInlineSize\|kPrefixSize" velox/type/StringView.h
echo -e "\n=== Look for how ClpArchiveJsonStringVectorLoader handles strings ==="
cat velox/connectors/clp/search_lib/archive/ClpArchiveJsonStringVectorLoader.cpp
echo -e "\n=== Check what generate_json_string returns ==="
rg -nP --type=cpp -B 3 -A 10 'generate_json_string|string generate_json' velox/connectors/clp/
echo -e "\n=== Check if StringView constructor copies for small strings ==="
cat velox/type/StringView.h | grep -B 5 -A 25 "StringView(const char\* data, int32_t len)"
Length of output: 4857
@wraymo, thank you for the clarification! You're absolutely right. Looking at the pattern across the codebase, I can see this same approach is used consistently in multiple places (ClpArchiveJsonStringVectorLoader.cpp, ClpIrVectorLoader.cpp, and even in test files). The widespread usage and your confirmation indicate that FlatVector<StringView>::set() does indeed handle copying the string data appropriately, likely through Velox's internal string buffer management.
For inline strings (≤12 bytes), StringView copies the data directly into its internal buffer, and for larger strings, the vector's buffer management ensures the data is preserved. My apologies for the false alarm on the use-after-free concern.
| TEST_F(ClpConnectorTest, test1JsonString) { | ||
| const std::shared_ptr<std::string> kqlQuery = nullptr; | ||
| auto plan = PlanBuilder() | ||
| .startTableScan() | ||
| .outputType( | ||
| ROW({"requestId", "__json_string", "method"}, | ||
| {VARCHAR(), VARCHAR(), VARCHAR()})) | ||
| .tableHandle(std::make_shared<ClpTableHandle>( | ||
| kClpConnectorId, "test_1")) | ||
| .assignments({ | ||
| {"requestId", | ||
| std::make_shared<ClpColumnHandle>( | ||
| "requestId", "requestId", VARCHAR())}, | ||
| {"__json_string", | ||
| std::make_shared<ClpColumnHandle>( | ||
| "__json_string", "__json_string", VARCHAR())}, | ||
| {"method", | ||
| std::make_shared<ClpColumnHandle>( | ||
| "method", "method", VARCHAR())}, | ||
| }) | ||
| .endTableScan() | ||
| .filter("method = 'GET'") | ||
| .planNode(); | ||
|
|
||
| const auto methodVector = makeFlatVector<StringView>({ | ||
| "GET", | ||
| "GET", | ||
| "GET", | ||
| "GET", | ||
| "GET", | ||
| }); | ||
| auto output = getResults( | ||
| plan, | ||
| {makeClpSplit( | ||
| getExampleFilePath("test_1.clps"), | ||
| ClpConnectorSplit::SplitType::kArchive, | ||
| kqlQuery)}); | ||
| auto expected = makeRowVector( | ||
| {// requestId | ||
| makeFlatVector<StringView>( | ||
| {"req-100", "req-105", "req-107", "req-109", "req-102"}), | ||
| // __json_string | ||
| makeFlatVector<StringView>({ | ||
| R"({"timestamp":"2025-04-30T08:45:00Z","requestId":"req-100","userId":"user201","method":"GET","path":"/api/users/1","responseTimeMs":25,"status":200})", | ||
| R"({"timestamp":"2025-04-30T08:45:25Z","requestId":"req-105","userId":"user204","method":"GET","path":"/api/dashboard","responseTimeMs":155,"status":200})", | ||
| R"({"timestamp":"2025-04-30T08:45:35Z","requestId":"req-107","userId":"user202","method":"GET","path":"/api/users/2/details","responseTimeMs":41,"status":200})", | ||
| R"({"timestamp":"2025-04-30T08:45:45Z","requestId":"req-109","userId":"user203","method":"GET","path":"/api/products?category=books","responseTimeMs":88,"status":200})", | ||
| R"({"timestamp":"2025-04-30T08:45:10Z","requestId":"req-102","method":"GET","path":"/public/products","responseTimeMs":18,"status":200,"userId":null})", | ||
| }), | ||
| // method | ||
| methodVector}); | ||
| test::assertEqualVectors(expected, output); | ||
|
|
||
| auto irOutput = getResults( | ||
| plan, | ||
| {makeClpSplit( | ||
| getExampleFilePath("test_1_ir.clp.zst"), | ||
| ClpConnectorSplit::SplitType::kIr, | ||
| kqlQuery)}); | ||
| auto irExpected = makeRowVector( | ||
| {// requestId | ||
| makeFlatVector<StringView>( | ||
| {"req-100", "req-102", "req-105", "req-107", "req-109"}), | ||
| // __json_string | ||
| makeFlatVector<StringView>({ | ||
| R"({"method":"GET","path":"/api/users/1","requestId":"req-100","responseTimeMs":25,"status":200,"timestamp":"2025-04-30T08:45:00Z","userId":"user201"})", | ||
| R"({"method":"GET","path":"/public/products","requestId":"req-102","responseTimeMs":18,"status":200,"timestamp":"2025-04-30T08:45:10Z","userId":null})", | ||
| R"({"method":"GET","path":"/api/dashboard","requestId":"req-105","responseTimeMs":155,"status":200,"timestamp":"2025-04-30T08:45:25Z","userId":"user204"})", | ||
| R"({"method":"GET","path":"/api/users/2/details","requestId":"req-107","responseTimeMs":41,"status":200,"timestamp":"2025-04-30T08:45:35Z","userId":"user202"})", | ||
| R"({"method":"GET","path":"/api/products?category=books","requestId":"req-109","responseTimeMs":88,"status":200,"timestamp":"2025-04-30T08:45:45Z","userId":"user203"})", | ||
| }), | ||
| // method | ||
| methodVector}); | ||
| test::assertEqualVectors(irExpected, irOutput); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make JSON assertions resilient to key-ordering.
Comparing full JSON strings is brittle. Parse and assert on fields (or compare parsed objects) to avoid failures if key order changes.
Example approach:
- Parse with folly::parseJson and compare selected keys (path, method, requestId, status).
- Or parse and compare to an expected folly::dynamic built from fields.
Minimal change: additionally assert that each value is valid JSON and contains required keys.
🤖 Prompt for AI Agents
velox/connectors/clp/tests/ClpConnectorTest.cpp lines 210-285: the test
currently compares raw JSON strings which is brittle to key ordering; update the
assertions to parse each __json_string with folly::parseJson and compare
selected fields (e.g., requestId, method, path, status) or compare to an
expected folly::dynamic constructed from those fields instead of the whole
string; for both the archive and IR result vectors iterate rows, parse the JSON,
assert parsing succeeded, assert required keys exist, and assert the selected
values equal the expected values (keep the existing requestId and method vector
comparisons but replace the full-string __json_string equality with these
parsed-field checks).
gibber9809
left a comment
gibber9809
left a comment
There was a problem hiding this comment.
Nice work! Apologies about the delay on the review.
Mostly a small style comment and a few questions.
| for (auto const& column : outputColumns_) { | ||
| for (size_t i = 0; i < outputColumns_.size(); i++) { | ||
| auto const& column = outputColumns_[i]; | ||
| if ("__json_string" == column.name) { |
There was a problem hiding this comment.
Could we replace "__json_string" with a constexpr constant throughout the code?
Also, is it ever possible for a user's column to have this name, or is that protected against somehow?
There was a problem hiding this comment.
Unfortunately, the user can use any column names if they are quoted. There doesn't seem to be a special column name that we can use exclusively for the json column.
There was a problem hiding this comment.
Is it possible to handle this by giving the output column a special "full JSON string" type then? If so could we add that as a backlog issue to fix this up later? Otherwise fine with this approach for now.
There was a problem hiding this comment.
Sorry, I missed the point of using a constant. Just pushed the fix. Yeah, we could do it internally, but the problem is the presto coordinator side. Probably we should add a field in ClpColumnHandle to indicate if it's a json string column.
There was a problem hiding this comment.
I think that's worth adding to the backlog then -- I think it makes sense to use a more robust approach since it should be possible.
| auto& reader = | ||
| archiveReader_->read_schema_table(currentSchemaId_, false, false); | ||
| reader.initialize_filter_with_column_map(queryRunner_.get()); | ||
| schemaReader_ = |
There was a problem hiding this comment.
Since we're passing this pointer to the JSON string vector loader, and the result of read_schema_table is invalidated by the next call to read_schema_table, I just want to double check that we're handling the lifetimes correctly.
Can we ever call read_schema_table multiple times while hanging onto this pointer?
There was a problem hiding this comment.
That's a good question. I think the situation is similar to how we handle ColumnReader, since it's also stored in the vector loader. From my understanding, Velox fully processes each input vector before moving on to the next one. For every vector we return, its data is confined within a single schema table — it never spans across multiple ones. So this approach should be fine.
gibber9809
left a comment
gibber9809
left a comment
There was a problem hiding this comment.
LGTM. Title is fine as well. Take a look my last comment about potentially changing the way we indicate full JSON string serialization is required, but I don't want to block this PR based on that.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp (2)
103-110: Avoid copying projectedColumns and initialise indices here.Minor: getProjectedColumns() returns a vector; copying it is unnecessary. Bind by const reference and initialise indices here (see paired diff in fetchNext() comment).
Apply this diff (if you didn’t apply the combined patch already):
- auto projectedColumns = getProjectedColumns(); + const auto& projectedColumns = getProjectedColumns(); + columnIndex_ = 0; + projectedColumnIndex_ = 0;
220-224: Duplicate timestamp index evaluation (minor).EvaluateTimestampIndex is executed twice (Lines 139‑143 and again here). Unless required due to state changes between calls, consider removing the second run to save work.
- EvaluateTimestampIndex timestamp_index(timestampDict); - if (EvaluatedValue::False == timestamp_index.run(expr_)) { - VLOG(2) << "No matching timestamp ranges for query '" << query_ << "'"; - return ErrorCode::InvalidTimestampRange; - }
♻️ Duplicate comments (3)
velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp (1)
205-216: Replace C++20contains()with C++17-compatible code.Line 206 uses
std::unordered_set::contains()which requires C++20, but Velox builds with C++17.Apply this diff:
- const bool isJsonString = jsonStringColumnIndices_.contains(columnIndex_); + const bool isJsonString = + jsonStringColumnIndices_.find(columnIndex_) != jsonStringColumnIndices_.end();velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp (2)
75-78: Schema reader pointer lifetime through LazyVector loaders needs a hard guarantee.schemaReader_ stores the address of ArchiveReader::read_schema_table(...). That object is documented to be invalidated by subsequent read_schema_table calls. Since the pointer is captured by LazyVector loaders, please confirm that no code path calls read_schema_table again until all LazyVectors produced for the current schema are fully materialised and released. If that guarantee is not absolute, pass a stable handle (e.g., archiveReader_ + currentSchemaId_) to the loader and reacquire the schema table inside the loader per load.
Would you like me to sketch a minimal change to make the loader hold (archiveReader_, currentSchemaId_) instead of a raw pointer?
23-26: Good move to centralise the marker name.Using ClpColumnHandle::jsonStringColumnName_ removes string literals and keeps things consistent. Given prior discussion about potential name collisions with user columns, let’s track a follow‑up to migrate to a dedicated logical “full JSON string” type in the handle/typing layer. I can open a backlog item if helpful.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
velox/connectors/clp/ClpColumnHandle.h(1 hunks)velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp(7 hunks)velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp (2)
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.h (2)
projectedColumnIndex_(69-69)columnIndex_(68-68)velox/connectors/clp/search_lib/archive/ClpQueryRunner.h (1)
vector(76-78)
velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp (2)
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp (2)
fetchNext(48-93)fetchNext(48-48)velox/connectors/clp/search_lib/ir/ClpIrCursor.h (2)
columnIndex_(91-91)projectedColumnIndex_(92-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (7)
velox/connectors/clp/ClpColumnHandle.h (1)
45-46: LGTM!The inline static constant is correctly defined and appropriately scoped as public for use across the CLP connector components.
velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp (4)
21-21: LGTM!The new includes are necessary for JSON string column support.
Also applies to: 23-23
31-32: LGTM!Resetting both index counters at the start of
fetchNextis correct and consistent with the archive cursor pattern.
126-164: LGTM!The logic correctly identifies
__json_stringcolumns by name, records their indices inoutputColumns_, and excludes them from the projected column list passed to the query handler.
65-69: Consider more precise validation.The validation checks that resolved columns plus JSON string columns don't exceed total columns, but
projectedColumnIdxNodeIdsMap_may not contain all projected columns (only resolved ones). A more precise check would verify that the number of projected columns (non-JSON-string) plus JSON string columns equals the total column count.Example:
- VELOX_CHECK_LE( - projectedColumnIdxNodeIdsMap_.size() + jsonStringColumnIndices_.size(), + const size_t projectedColumnCount = outputColumns_.size() - jsonStringColumnIndices_.size(); + VELOX_CHECK_EQ( + projectedColumnCount + jsonStringColumnIndices_.size(), outputColumns_.size(), - "Resolved node-id map size ({}) must not exceed projected columns and json string columns ({})", - projectedColumnIdxNodeIdsMap_.size() + jsonStringColumnIndices_.size(), + "Projected columns ({}) plus JSON string columns ({}) must equal total columns ({})", + projectedColumnCount, + jsonStringColumnIndices_.size(), outputColumns_.size());Likely an incorrect or invalid review comment.
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp (2)
246-253: Row children preallocation LGTM.children.reserve(numChildren) and iterating with uint32_t are appropriate here.
274-281: Bounds check and reader/type lookup LGTM.The bounds check before dereferencing projectedColumns and mapping via projectedColumnIndex_ is correct.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp (1)
206-206: Build break:unordered_set::containsrequires C++20.Velox builds with C++17. Replace
contains(...)withfind(...) != end().Apply this diff to fix the compatibility issue:
- if (jsonStringColumnIndices_.contains(columnIndex_)) { + if (jsonStringColumnIndices_.find(columnIndex_) != jsonStringColumnIndices_.end()) {velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp (4)
75-78: Pointer lifetime: document invariants for schemaReader_.schemaReader_ stores the address of ArchiveReader::read_schema_table(...). Ensure LazyVectors are fully materialised before any subsequent read_schema_table() call that would invalidate this pointer. Add a brief comment/assert to encode this invariant.
50-51: Reset column counters at vector creation (not in fetchNext) to avoid state leaks.fetchNext() may be called once per batch, but createVector() can be called multiple times; keeping resets here is brittle. Move resets to the start of createVector(), and bind projectedColumns as a const reference to avoid an extra copy.
Apply these diffs:
--- a/velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp +++ b/velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp @@ filteredRowIndices_->clear(); - projectedColumnIndex_ = 0; - columnIndex_ = 0;--- a/velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp +++ b/velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp @@ VectorPtr ClpArchiveCursor::createVector( memory::MemoryPool* pool, const TypePtr& vectorType, size_t vectorSize) { - auto projectedColumns = getProjectedColumns(); + const auto& projectedColumns = getProjectedColumns(); + // Reset per-call to guarantee deterministic mapping regardless of how + // many times createVector() is invoked for the current batch. + projectedColumnIndex_ = 0; + columnIndex_ = 0; VELOX_CHECK_EQ( projectedColumns.size() + jsonStringColumnIndices_.size(), outputColumns_.size(), "Projected columns size {} does not match fields size {}", projectedColumns.size(), outputColumns_.size()); return createVectorHelper(pool, vectorType, vectorSize, projectedColumns); }Also applies to: 103-111
155-160: Fail fast if “__json_string” isn’t declared as String at planning time.Without this, mismatched types surface later during materialisation. Add a check when detecting the marker column.
@@ - for (size_t i = 0; i < outputColumns_.size(); i++) { + for (size_t i = 0; i < outputColumns_.size(); i++) { auto const& column = outputColumns_[i]; if (ClpColumnHandle::jsonStringColumnName_ == column.name) { + VELOX_CHECK_EQ( + column.type, + ColumnType::String, + "__json_string must be declared as a String/VARCHAR column"); jsonStringColumnIndices_.insert(i); continue; }
262-271: Runtime guard: “__json_string” must materialise as VARCHAR.Add a TypeKind check before constructing the JSON-string LazyVector to improve diagnostics and prevent undefined behaviour.
@@ - if (jsonStringColumnIndices_.contains(columnIndex_)) { + if (jsonStringColumnIndices_.contains(columnIndex_)) { + VELOX_CHECK_EQ( + vectorType->kind(), + TypeKind::VARCHAR, + "__json_string columns must materialise as VARCHAR"); ++columnIndex_; return std::make_shared<LazyVector>( pool, vectorType, vectorSize, std::make_unique<ClpArchiveJsonStringVectorLoader>( schemaReader_, filteredRowIndices_), std::move(vector)); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp(7 hunks)velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp (2)
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp (2)
fetchNext(48-93)fetchNext(48-48)velox/connectors/clp/search_lib/ir/ClpIrCursor.h (2)
columnIndex_(91-91)projectedColumnIndex_(92-92)
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp (2)
velox/connectors/clp/search_lib/archive/ClpArchiveCursor.h (2)
projectedColumnIndex_(69-69)columnIndex_(68-68)velox/connectors/clp/search_lib/archive/ClpQueryRunner.h (1)
vector(76-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (6)
velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp (5)
21-21: LGTM: Includes are necessary for JSON string support.The new includes provide the column name constant and the JSON string vector loader class required by the new functionality.
Also applies to: 23-23
31-32: LGTM: Counter reset aligns with new indexing scheme.Resetting both
columnIndex_andprojectedColumnIndex_at the start of each fetch is correct and consistent with the parallel implementation inClpArchiveCursor.cpp.
65-69: LGTM: Validation correctly accounts for JSON string columns.The bounds check and error message properly reflect the new distinction between projected columns and JSON string columns.
126-164: LGTM: JSON string column detection logic is correct.The method properly identifies
__json_stringcolumns by name, records their indices injsonStringColumnIndices_, and excludes them from the projection list passed to the query handler. The signature change to non-const is necessary since the method now modifies member state.
220-230: Indexing logic is correct.The use of
columnIndex_to accessoutputColumns_[columnIndex_](line 220) andprojectedColumnIndex_for the map lookup (line 222) properly addresses the indexing bug flagged in previous reviews. Both counters are correctly incremented after processing a non-JSON-string column.velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp (1)
23-26: Includes look good.Using ClpColumnHandle::jsonStringColumnName_ and adding the JSON-string loader header is the right direction.
2 participants
Description
This PR adds support for CLP JSON string serialization, enabling the
clp_get_json_stringUDF to return serialized JSON strings in Presto.Specifically:
__json_stringto represent columns that require serialized JSON output.ClpArchiveJsonStringVectorLoaderandClpIrJsonStringVectorLoaderto initialize the JSON serializer and perform on-demand serialization when creating vectors for these columns.Checklist
breaking change.
Validation performed
Did an end-to-end testing with a sample dataset
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores