Repository navigation
feat: Add C++ Presto Worker plugin. - #4
Merged
20001020ycx merged 12 commits intoMay 25, 2026
Merged
20001020ycx merged 12 commits into
20001020ycx merged 12 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CLP Presto connector plugin: Presto protocol types/deserializers, Presto→Velox translation, Velox connector and data source, archive and IR search cursors/vector loaders, S3 auth handling, CMake build wiring, and comprehensive GoogleTest suites with NDJSON fixtures. ChangesCLP Presto Connector Plugin
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@velox-connector/src/connector/ClpConfig.cpp`:
- Line 30: Replace the negation operator usage in the VELOX_CHECK call so it
uses an explicit comparison: change VELOX_CHECK(!upperValue.empty()); to
VELOX_CHECK(false == upperValue.empty()); ensuring the check on upperValue (the
variable in ClpConfig.cpp) follows the project's coding guideline against using
'!' directly.
In `@velox-connector/src/connector/ClpConfig.h`:
- Around line 17-18: ClpConfig.h uses std::shared_ptr in the public API
(ClpConfig constructor parameter, the get...() methods that return shared_ptr,
and the shared_ptr member variables) but does not include <memory>; add `#include`
<memory> to this header so the declaration of std::shared_ptr is always
available and the header is not order-dependent.
In `@velox-connector/src/connector/ClpConnectorSplit.h`:
- Line 39: The ternary uses implicit shared_ptr boolean conversion for
kqlQuery_; change it to an explicit nullptr comparison to follow guidelines —
e.g. replace the condition with (kqlQuery_ != nullptr) ? *kqlQuery_ : "<null>"
so ClpConnectorSplit.h uses an explicit check against nullptr for kqlQuery_.
- Around line 24-32: The constructor ClpConnectorSplit currently does
static_cast<SplitType>(type) without validation; validate that the incoming int
is a valid SplitType value (e.g., 0 or 1) before assigning to type_. If the
value is invalid, either throw a clear std::invalid_argument (or similar)
mentioning connectorId/path, or set type_ to a safe default and log the error;
update the initializer to perform the check (in the ctor body) and only assign
type_ after validation so toString() and any switches on type_ never see an
out-of-range enum.
In `@velox-connector/src/connector/ClpDataSource.cpp`:
- Around line 37-40: The dynamic cast into clpTableHandle in the ClpDataSource
constructor is performed but never used or validated; replace this dead cast
with a proper null-check so the constructor rejects incompatible handles: after
auto clpTableHandle = std::dynamic_pointer_cast<const
ClpTableHandle>(tableHandle); add VELOX_CHECK_NOT_NULL(clpTableHandle) (or
equivalent check) to validate the cast and fail early, ensuring you reference
clpTableHandle and ClpTableHandle in the constructor validation path;
alternatively, if you truly don't need the casted handle, remove the
dynamic_pointer_cast line entirely.
- Around line 127-132: Change the conditional that checks clpSplit->kqlQuery_ to
use the coding guideline form instead of negation: replace the `if
(pushDownQuery && !pushDownQuery->empty())` check with one that keeps the
pointer check and uses `false == pushDownQuery->empty()` (i.e., `pushDownQuery`
and `false == pushDownQuery->empty()`), leaving the cursor_->executeQuery(...)
calls with the same arguments (pushDownQuery / "*" and fields_).
- Around line 102-114: In ClpDataSource::addSplit, guard the result of
std::dynamic_pointer_cast<ClpConnectorSplit> (clpSplit) before dereferencing it:
replace the direct use of clpSplit->path_ with a null-check using the project’s
convention (e.g., VELOX_CHECK_NOT_NULL(clpSplit) or an explicit if that
logs/errors and returns) so a ConnectorSplit of the wrong runtime type won’t
cause an NPE; ensure the check occurs immediately after the cast and before any
use of clpSplit, then proceed to set splitPath/inputSource as before.
- Around line 135-146: ClpDataSource::next currently returns nullptr when
cursor_->getNumFilteredRows() == 0 which prematurely ends IR splits; change
logic to only return nullptr for EOF by asking IR cursors whether the underlying
stream is completed (use
irDeserializer_->is_stream_completed()/ClpIrCursor::deserialize semantics)
instead of unconditionally returning on zero filtered rows—i.e., if cursor is an
IR-type, check its is_stream_completed() before signaling end. Also fix
ClpIrCursor::fetchNext to stop returning the cumulative
irDeserializer_->get_num_log_events_deserialized() value: store the previous
cumulative count inside ClpIrCursor and return the delta (current minus
previous) so completedRows_ in ClpDataSource::next is incremented by the
per-call scanned count (matching ClpArchiveCursor::fetchNext behavior).
In `@velox-connector/src/connector/CMakeLists.txt`:
- Around line 10-12: The target_compile_features call for target
"presto_clp_connector" is currently set to PRIVATE which won't propagate C++20
to consumers even though the target exposes PUBLIC include directories; change
the feature scope to PUBLIC by updating the target_compile_features invocation
for presto_clp_connector so that cxx_std_20 is propagated, ensuring any public
headers using C++20 compile correctly for downstream targets (also verify no
duplicate/conflicting compile feature settings elsewhere for
presto_clp_connector).
In `@velox-connector/src/connector/search_lib/archive/ClpArchiveVectorLoader.cpp`:
- Around line 89-130: populateTimestampData writes timestamp values with
vector->set(...) but never clears the null bit, so reused result vectors can
keep rows marked null; update populateTimestampData (the loop using
filteredRowIndices_, columnReader_ and
convertNanosecondEpochToVeloxTimestamp/convertToVeloxTimestamp) to explicitly
mark the slot as non-null after a successful write (i.e., call the vector
null-clear/mark-non-null API for vectorIndex immediately after each
vector->set(...) in every branch — Timestamp, Float, FormattedFloat,
DictionaryFloat, Integer, and the fallback DeprecatedDateString reader).
In `@velox-connector/src/connector/search_lib/archive/ClpQueryRunner.cpp`:
- Around line 50-52: Change the boolean negation to the repo's required style by
replacing the use of "!foundReader" with "false == foundReader" in
ClpQueryRunner.cpp; specifically update the conditional that checks foundReader
(the if block that calls projectedColumns_.push_back(nullptr)) so the condition
reads false == foundReader while leaving the body unchanged.
In `@velox-connector/src/connector/search_lib/ClpPackageS3AuthProvider.cpp`:
- Line 25: Replace the negated call in the VELOX_CHECK invocation so it uses an
explicit comparison: change the condition that currently reads using
!splitPath.empty() to use false == splitPath.empty() in the VELOX_CHECK call
(the check that ensures splitPath is not empty in ClpPackageS3AuthProvider.cpp).
- Line 36: The VELOX_CHECK uses a negation operator on endPoint_
(VELOX_CHECK(!endPoint_.empty(), fmt::format("{} cannot be empty", kEndPoint)));
replace the negation with an explicit comparison per guideline by changing the
condition to use false == endPoint_.empty() while leaving the existing error
message and symbols (endPoint_, kEndPoint, VELOX_CHECK) intact.
- Around line 48-50: Replace the negation operator in the VELOX_CHECK condition
so it uses an explicit comparison: in ClpPackageS3AuthProvider.cpp change the
check that currently reads VELOX_CHECK(!secretAccessKey.empty(), fmt::format("{}
cannot be empty", kSecretAccessKey)) to use false == secretAccessKey.empty()
(keeping the same fmt::format message and kSecretAccessKey symbol) so the
VELOX_CHECK invocation uses an explicit comparison rather than the ! operator.
- Line 55: Change the conditional that uses the negation operator on
sessionToken (currently written as !sessionToken.empty()) to an explicit
comparison using false == sessionToken.empty() so the if condition reads with
the preferred explicit comparison; update the check in the method/class where
sessionToken is evaluated (ClpPackageS3AuthProvider / the function containing
the if) accordingly.
- Around line 46-47: Replace the negation operator in the VELOX_CHECK condition
so it uses an explicit comparison: change the condition in the VELOX_CHECK that
currently reads using !accessKeyId.empty() to use false == accessKeyId.empty()
while keeping the same message using kAccessKeyId; this updates the check around
accessKeyId to follow the coding guideline without altering behavior.
In `@velox-connector/src/connector/search_lib/ClpS3AuthProviderBase.cpp`:
- Around line 57-66: The Windows branch currently calls
_putenv(fmt::format("{}=", keyCStr)) which passes a pointer to a temporary
string and leads to dangling pointer UB; replace that call with
_putenv_s(keyCStr, "") so the runtime manages the buffer lifetime (assign the
return to err like the other branches), keep the Unix/macOS branch using
unsetenv(keyCStr) and leave the VELOX_CHECK_EQ(0, err) validation in place;
update the code around symbols _putenv, _putenv_s, unsetenv, keyCStr, and err
accordingly.
- Around line 48-51: The sanity check currently calls std::strcmp(valueCStr,
valueForCheck) without ensuring valueForCheck is non-null, risking a crash if
getenv returns nullptr; update the code around valueForCheck (the result of
std::getenv(keyCStr)) to first assert/value-check that valueForCheck != nullptr
(e.g., VELOX_CHECK(valueForCheck != nullptr) or
VELOX_CHECK_NOT_NULL(valueForCheck)) and only then call std::strcmp, finally
keeping the VELOX_CHECK_EQ comparison of strcmp(...) == 0 to validate the value
match.
In `@velox-connector/src/connector/search_lib/CMakeLists.txt`:
- Around line 1-8: The STATIC library clp_search (and its subdirs
clp_search_archive / clp_search_ir) must be built PIC-safe so shared plugins can
link; update the CMakeLists to enforce position-independent code for the target
(e.g., set the target property POSITION_INDEPENDENT_CODE ON for clp_search or
add -fPIC to its compile options) so the static libs are PIC regardless of
global settings.
In `@velox-connector/src/connector/search_lib/ir/ClpIrCursor.cpp`:
- Around line 88-93: There are two boolean-negation checks using the `!`
operator that violate the coding guideline; replace `!queryHandlerResult` with
`false == queryHandlerResult` in the creation/check of the `queryHandlerResult`
(the block that currently logs "Failed to create query handler for
deserialization.") and similarly replace the other `!` check in the later block
(around the code handling the second query handler check in the same ClpIrCursor
logic) with `false == <expression>` so both occurrences use the `false == ...`
form; keep the existing logging and return behavior intact.
- Around line 102-110: Remove the unreachable null check on irReaderZstdWrapper_
and wrap the Decompressor creation/open sequence in exception handling: allocate
irReaderZstdWrapper_ with std::make_shared as before, then call
irReaderZstdWrapper_->open(*irReader_, cReaderBufferSize) inside a try block,
catch std::exception (or std::bad_alloc separately if desired), log a clear
failure message via VLOG(2) including splitPath_ and the exception.what(), and
return ErrorCode::InternalError on catch; ensure the original misleading log
before open() is removed or moved to the catch so errors reflect the actual
failure of Decompressor::open.
In `@velox-connector/src/connector/search_lib/ir/ClpIrUnitHandler.h`:
- Line 25: The header ClpIrUnitHandler.h erroneously includes itself; remove the
self-include directive `#include "connector/search_lib/ir/ClpIrUnitHandler.h"`
from that file so it no longer causes recursive inclusion and compilation
errors, and confirm the file has proper include guards or `#pragma once` to
prevent multiple inclusion; also scan the top of ClpIrUnitHandler.h for any
other accidental self-references and delete them.
In `@velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp`:
- Around line 30-33: Replace the negated boolean checks that use the '!'
operator with the project's preferred style using 'false == <expression>' in
ClpIrVectorLoader.cpp: change instances like '!isResolved_' and
'!decodeResult.has_value()' (and any similar checks in the same file around the
other noted blocks) to use 'false == isResolved_' and 'false ==
decodeResult.has_value()' (or 'false == <same-expression>') so the checks in the
loop over rows and the other affected blocks follow the repo convention.
- Around line 102-115: The Timestamp branch in ClpIrVectorLoader.cpp
(ColumnType::Timestamp case) sets values into timestampVector but never clears
the null bit, so timestamps remain NULL; after each successful
timestampVector->set(...) call (both the double and int64_t branches) add a call
to vector->setNull(vectorIndex, false) to reset the null indicator, mirroring
the other type branches (e.g., Boolean, Integer, Float, String) so the written
timestamp is treated as non-null.
- Around line 117-153: The Array branch currently resizes the shared elements
flat vector per-row and always sets each row's offset to 0, corrupting multi-row
results; change this to append elements to the shared elements buffer using a
persistent cumulative index (e.g., a local persistent size_t
elementsOffset/nextElementIndex) so each row sets
arrayVector->setOffsetAndSize(vectorIndex, elementsOffset, rowCount) and then
increments elementsOffset by rowCount (grow or reserve the elements flat vector
rather than resetting it), and ensure you use elements->set(elementsOffset + i,
...) for each appended element; also replace the style-violating checks `if
(!decodeResult.has_value())` with the preferred `if (false ==
decodeResult.has_value())` in both decode branches.
In `@velox-connector/src/connector/tests/ClpConfigTest.cpp`:
- Around line 36-39: Normalize the unary negation checks to the project's
preferred style: replace uses of "!envStrings" with "nullptr == envStrings"
after GetEnvironmentStringsA() and replace uses of "!expectedValue.has_value()"
with "false == expectedValue.has_value()" (or equivalent comparison) in the test
helper; update both the envStrings check (symbol: envStrings /
GetEnvironmentStringsA) and the has_value check (symbol:
expectedValue.has_value) around the other occurrence referenced (lines ~167-171)
so they conform to the coding guideline.
In `@velox-connector/src/connector/tests/ClpConnectorTest.cpp`:
- Around line 80-83: getExampleFilePath currently uses fs::current_path() making
test fixture lookup CWD-dependent; change it to resolve examples from a stable,
build-time path supplied by the build system (e.g. a compile-time macro like
EXAMPLES_DIR or a const string defined via -DEXAMPLES_DIR="...") instead of
fs::current_path(), and update getExampleFilePath to concatenate that
macro/constant with the relative filePath so tests find examples regardless of
working directory; ensure the macro is documented in CMake and provide a
sensible fallback or static path for local dev if desired.
In `@velox-connector/src/protocol/presto_protocol_clp.h`:
- Around line 35-37: operator< currently uses dynamic_cast<const
ClpColumnHandle&>(o) which will throw std::bad_cast if o is not a
ClpColumnHandle; replace the unsafe cast with a safe RTTI check: use
dynamic_cast<const ClpColumnHandle*>(&o) and if the result is non-null compare
columnName to that->columnName, otherwise decide a deterministic fallback (e.g.
compare std::type_index(typeid(*this)) vs std::type_index(typeid(o)) or
assert/log and return false) so the operator never throws; update the
implementation inside the ClpColumnHandle::operator< to perform this null-check
and use the chosen fallback behavior consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf19eaf2-f0b2-4180-bc7a-a0848b5d91c0
⛔ Files ignored due to path filters (4)
velox-connector/plugin.mapis excluded by!**/*.mapvelox-connector/src/connector/tests/examples/test_1_ir.clp.zstis excluded by!**/*.zstvelox-connector/src/connector/tests/examples/test_2_ir.clp.zstis excluded by!**/*.zstvelox-connector/src/connector/tests/examples/test_4_ir.clp.zstis excluded by!**/*.zst
📒 Files selected for processing (60)
velox-connector/.gitignorevelox-connector/CMakeLists.txtvelox-connector/src/CMakeLists.txtvelox-connector/src/ClpPluginEntry.cppvelox-connector/src/ClpPrestoToVeloxConnector.cppvelox-connector/src/ClpPrestoToVeloxConnector.hvelox-connector/src/connector/CMakeLists.txtvelox-connector/src/connector/ClpColumnHandle.hvelox-connector/src/connector/ClpConfig.cppvelox-connector/src/connector/ClpConfig.hvelox-connector/src/connector/ClpConnector.cppvelox-connector/src/connector/ClpConnector.hvelox-connector/src/connector/ClpConnectorSplit.hvelox-connector/src/connector/ClpDataSource.cppvelox-connector/src/connector/ClpDataSource.hvelox-connector/src/connector/ClpTableHandle.cppvelox-connector/src/connector/ClpTableHandle.hvelox-connector/src/connector/search_lib/BaseClpCursor.cppvelox-connector/src/connector/search_lib/BaseClpCursor.hvelox-connector/src/connector/search_lib/CMakeLists.txtvelox-connector/src/connector/search_lib/ClpPackageS3AuthProvider.cppvelox-connector/src/connector/search_lib/ClpPackageS3AuthProvider.hvelox-connector/src/connector/search_lib/ClpS3AuthProviderBase.cppvelox-connector/src/connector/search_lib/ClpS3AuthProviderBase.hvelox-connector/src/connector/search_lib/ClpTimestampsUtils.hvelox-connector/src/connector/search_lib/archive/CMakeLists.txtvelox-connector/src/connector/search_lib/archive/ClpArchiveCursor.cppvelox-connector/src/connector/search_lib/archive/ClpArchiveCursor.hvelox-connector/src/connector/search_lib/archive/ClpArchiveJsonStringVectorLoader.cppvelox-connector/src/connector/search_lib/archive/ClpArchiveJsonStringVectorLoader.hvelox-connector/src/connector/search_lib/archive/ClpArchiveVectorLoader.cppvelox-connector/src/connector/search_lib/archive/ClpArchiveVectorLoader.hvelox-connector/src/connector/search_lib/archive/ClpQueryRunner.cppvelox-connector/src/connector/search_lib/archive/ClpQueryRunner.hvelox-connector/src/connector/search_lib/ir/CMakeLists.txtvelox-connector/src/connector/search_lib/ir/ClpIrCursor.cppvelox-connector/src/connector/search_lib/ir/ClpIrCursor.hvelox-connector/src/connector/search_lib/ir/ClpIrJsonStringVectorLoader.cppvelox-connector/src/connector/search_lib/ir/ClpIrJsonStringVectorLoader.hvelox-connector/src/connector/search_lib/ir/ClpIrUnitHandler.hvelox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cppvelox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.hvelox-connector/src/connector/tests/CMakeLists.txtvelox-connector/src/connector/tests/ClpConfigTest.cppvelox-connector/src/connector/tests/ClpConnectorTest.cppvelox-connector/src/connector/tests/examples/test_1.clpsvelox-connector/src/connector/tests/examples/test_1.ndjsonvelox-connector/src/connector/tests/examples/test_2.clpsvelox-connector/src/connector/tests/examples/test_2.ndjsonvelox-connector/src/connector/tests/examples/test_3.clpsvelox-connector/src/connector/tests/examples/test_3.ndjsonvelox-connector/src/connector/tests/examples/test_4.ndjsonvelox-connector/src/connector/tests/examples/test_5.clpsvelox-connector/src/connector/tests/examples/test_5.ndjsonvelox-connector/src/connector/tests/examples/test_5.v0.5.0.clpsvelox-connector/src/protocol/CMakeLists.txtvelox-connector/src/protocol/ClpConnectorProtocol.cppvelox-connector/src/protocol/ClpConnectorProtocol.hvelox-connector/src/protocol/ClpProtocolTypes.cppvelox-connector/src/protocol/presto_protocol_clp.h
|
|
||
| ClpConfig::S3AuthProvider stringToS3AuthProvider(const std::string& strValue) { | ||
| auto upperValue = boost::algorithm::to_upper_copy(strValue); | ||
| VELOX_CHECK(!upperValue.empty()); |
There was a problem hiding this comment.
Replace negation operator with explicit comparison.
The expression !upperValue.empty() uses the negation operator. As per coding guidelines, prefer false == upperValue.empty() for consistency.
📝 Proposed fix
- VELOX_CHECK(!upperValue.empty());
+ VELOX_CHECK(false == upperValue.empty());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| VELOX_CHECK(!upperValue.empty()); | |
| VELOX_CHECK(false == upperValue.empty()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/connector/ClpConfig.cpp` at line 30, Replace the negation
operator usage in the VELOX_CHECK call so it uses an explicit comparison: change
VELOX_CHECK(!upperValue.empty()); to VELOX_CHECK(false == upperValue.empty());
ensuring the check on upperValue (the variable in ClpConfig.cpp) follows the
project's coding guideline against using '!' directly.
Comment on lines
+17
to
+18
| #pragma once | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "ClpConfig.h" -type fRepository: y-scope/clp-plugin-presto-connector
Length of output: 123
🏁 Script executed:
cat -n ./velox-connector/src/connector/ClpConfig.hRepository: y-scope/clp-plugin-presto-connector
Length of output: 2035
Include <memory> in this public header.
ClpConfig exposes std::shared_ptr in the interface (constructor parameter at line 41, method return types at lines 43 and 49, and member variables at lines 52–53), but the header never includes <memory>. This makes the header order-dependent and will break any translation unit that includes it without some other file pulling <memory> in first.
Proposed fix
`#pragma` once
+
+#include <memory>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #pragma once | |
| `#pragma` once | |
| `#include` <memory> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/connector/ClpConfig.h` around lines 17 - 18, ClpConfig.h
uses std::shared_ptr in the public API (ClpConfig constructor parameter, the
get...() methods that return shared_ptr, and the shared_ptr member variables)
but does not include <memory>; add `#include` <memory> to this header so the
declaration of std::shared_ptr is always available and the header is not
order-dependent.
Comment on lines
+24
to
+32
| ClpConnectorSplit( | ||
| const std::string& connectorId, | ||
| const std::string& path, | ||
| const int type, | ||
| std::shared_ptr<std::string> kqlQuery) | ||
| : connector::ConnectorSplit(connectorId), | ||
| path_(path), | ||
| type_(static_cast<SplitType>(type)), | ||
| kqlQuery_(std::move(kqlQuery)) {} |
There was a problem hiding this comment.
Validate the type parameter before casting to enum.
The static_cast<SplitType>(type) on line 31 does not validate that type is a valid enum value (0 or 1). If an invalid integer is passed (e.g., from malformed protocol data), the cast produces undefined behaviour and could cause incorrect output in toString() or unexpected behaviour in code that switches on type_.
🛡️ Proposed fix adding validation
ClpConnectorSplit(
const std::string& connectorId,
const std::string& path,
const int type,
std::shared_ptr<std::string> kqlQuery)
: connector::ConnectorSplit(connectorId),
path_(path),
- type_(static_cast<SplitType>(type)),
+ type_([type]() {
+ VELOX_CHECK(
+ type == static_cast<int>(SplitType::kArchive) ||
+ type == static_cast<int>(SplitType::kIr),
+ "Invalid split type: {}", type);
+ return static_cast<SplitType>(type);
+ }()),
kqlQuery_(std::move(kqlQuery)) {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ClpConnectorSplit( | |
| const std::string& connectorId, | |
| const std::string& path, | |
| const int type, | |
| std::shared_ptr<std::string> kqlQuery) | |
| : connector::ConnectorSplit(connectorId), | |
| path_(path), | |
| type_(static_cast<SplitType>(type)), | |
| kqlQuery_(std::move(kqlQuery)) {} | |
| ClpConnectorSplit( | |
| const std::string& connectorId, | |
| const std::string& path, | |
| const int type, | |
| std::shared_ptr<std::string> kqlQuery) | |
| : connector::ConnectorSplit(connectorId), | |
| path_(path), | |
| type_([type]() { | |
| VELOX_CHECK( | |
| type == static_cast<int>(SplitType::kArchive) || | |
| type == static_cast<int>(SplitType::kIr), | |
| "Invalid split type: {}", type); | |
| return static_cast<SplitType>(type); | |
| }()), | |
| kqlQuery_(std::move(kqlQuery)) {} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/connector/ClpConnectorSplit.h` around lines 24 - 32, The
constructor ClpConnectorSplit currently does static_cast<SplitType>(type)
without validation; validate that the incoming int is a valid SplitType value
(e.g., 0 or 1) before assigning to type_. If the value is invalid, either throw
a clear std::invalid_argument (or similar) mentioning connectorId/path, or set
type_ to a safe default and log the error; update the initializer to perform the
check (in the ctor body) and only assign type_ after validation so toString()
and any switches on type_ never see an out-of-range enum.
| "CLP Split: path: {}, type: {}, kqlQuery: {}", | ||
| path_, | ||
| type_ == SplitType::kArchive ? "Archive" : "Ir", | ||
| kqlQuery_ ? *kqlQuery_ : "<null>"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Prefer explicit nullptr comparison over implicit boolean conversion.
The expression kqlQuery_ ? *kqlQuery_ : "<null>" uses implicit boolean conversion of the shared_ptr. As per coding guidelines, prefer false == <expression> rather than !<expression>.
♻️ Suggested refactor
- kqlQuery_ ? *kqlQuery_ : "<null>");
+ nullptr != kqlQuery_ ? *kqlQuery_ : "<null>");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| kqlQuery_ ? *kqlQuery_ : "<null>"); | |
| nullptr != kqlQuery_ ? *kqlQuery_ : "<null>"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/connector/ClpConnectorSplit.h` at line 39, The ternary
uses implicit shared_ptr boolean conversion for kqlQuery_; change it to an
explicit nullptr comparison to follow guidelines — e.g. replace the condition
with (kqlQuery_ != nullptr) ? *kqlQuery_ : "<null>" so ClpConnectorSplit.h uses
an explicit check against nullptr for kqlQuery_.
Comment on lines
+37
to
+40
| auto clpTableHandle = | ||
| std::dynamic_pointer_cast<const ClpTableHandle>(tableHandle); | ||
| storageType_ = clpConfig->storageType(); | ||
| s3AuthProvider_ = clpConfig->s3AuthProvider(); |
There was a problem hiding this comment.
Dead clpTableHandle cast — either validate or remove.
clpTableHandle is produced via dynamic_pointer_cast<const ClpTableHandle> and then never used or checked. Either drop the cast entirely or follow the column-handle pattern and VELOX_CHECK_NOT_NULL it so the constructor refuses an incompatible table handle.
🛡️ Proposed fix
auto clpTableHandle =
std::dynamic_pointer_cast<const ClpTableHandle>(tableHandle);
+ VELOX_CHECK_NOT_NULL(
+ clpTableHandle,
+ "TableHandle must be an instance of ClpTableHandle");
storageType_ = clpConfig->storageType();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/connector/ClpDataSource.cpp` around lines 37 - 40, The
dynamic cast into clpTableHandle in the ClpDataSource constructor is performed
but never used or validated; replace this dead cast with a proper null-check so
the constructor rejects incompatible handles: after auto clpTableHandle =
std::dynamic_pointer_cast<const ClpTableHandle>(tableHandle); add
VELOX_CHECK_NOT_NULL(clpTableHandle) (or equivalent check) to validate the cast
and fail early, ensuring you reference clpTableHandle and ClpTableHandle in the
constructor validation path; alternatively, if you truly don't need the casted
handle, remove the dynamic_pointer_cast line entirely.
Comment on lines
+102
to
+115
| case ColumnType::Timestamp: { | ||
| auto timestampVector = vector->asFlatVector<Timestamp>(); | ||
| if (value->is<double>()) { | ||
| timestampVector->set( | ||
| vectorIndex, | ||
| convertToVeloxTimestamp(value->get_immutable_view<double>())); | ||
| } else if (value->is<int64_t>()) { | ||
| timestampVector->set( | ||
| vectorIndex, | ||
| convertToVeloxTimestamp(value->get_immutable_view<int64_t>())); | ||
| } else { | ||
| VELOX_FAIL("Unsupported timestamp type"); | ||
| } | ||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f 'ClpIrVectorLoader.cpp'Repository: y-scope/clp-plugin-presto-connector
Length of output: 145
🏁 Script executed:
wc -l velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cppRepository: y-scope/clp-plugin-presto-connector
Length of output: 149
🏁 Script executed:
sed -n '25,45p' velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cppRepository: y-scope/clp-plugin-presto-connector
Length of output: 824
🏁 Script executed:
sed -n '95,125p' velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cppRepository: y-scope/clp-plugin-presto-connector
Length of output: 1307
🏁 Script executed:
sed -n '65,130p' velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cppRepository: y-scope/clp-plugin-presto-connector
Length of output: 2597
🏁 Script executed:
grep -n "vector->setNull" velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cppRepository: y-scope/clp-plugin-presto-connector
Length of output: 314
Timestamp rows stay NULL in the IR loader – missing null bit reset.
Every case branch that successfully sets a value resets the null bit with vector->setNull(vectorIndex, false) (see Boolean at line 99, Integer at line 84, Float at line 92, String at line 77). The Timestamp case is missing this call, so converted timestamp values remain marked NULL despite being successfully set.
Proposed fix
case ColumnType::Timestamp: {
auto timestampVector = vector->asFlatVector<Timestamp>();
if (value->is<double>()) {
timestampVector->set(
vectorIndex,
convertToVeloxTimestamp(value->get_immutable_view<double>()));
} else if (value->is<int64_t>()) {
timestampVector->set(
vectorIndex,
convertToVeloxTimestamp(value->get_immutable_view<int64_t>()));
} else {
VELOX_FAIL("Unsupported timestamp type");
}
+ vector->setNull(vectorIndex, false);
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case ColumnType::Timestamp: { | |
| auto timestampVector = vector->asFlatVector<Timestamp>(); | |
| if (value->is<double>()) { | |
| timestampVector->set( | |
| vectorIndex, | |
| convertToVeloxTimestamp(value->get_immutable_view<double>())); | |
| } else if (value->is<int64_t>()) { | |
| timestampVector->set( | |
| vectorIndex, | |
| convertToVeloxTimestamp(value->get_immutable_view<int64_t>())); | |
| } else { | |
| VELOX_FAIL("Unsupported timestamp type"); | |
| } | |
| break; | |
| case ColumnType::Timestamp: { | |
| auto timestampVector = vector->asFlatVector<Timestamp>(); | |
| if (value->is<double>()) { | |
| timestampVector->set( | |
| vectorIndex, | |
| convertToVeloxTimestamp(value->get_immutable_view<double>())); | |
| } else if (value->is<int64_t>()) { | |
| timestampVector->set( | |
| vectorIndex, | |
| convertToVeloxTimestamp(value->get_immutable_view<int64_t>())); | |
| } else { | |
| VELOX_FAIL("Unsupported timestamp type"); | |
| } | |
| vector->setNull(vectorIndex, false); | |
| break; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp` around
lines 102 - 115, The Timestamp branch in ClpIrVectorLoader.cpp
(ColumnType::Timestamp case) sets values into timestampVector but never clears
the null bit, so timestamps remain NULL; after each successful
timestampVector->set(...) call (both the double and int64_t branches) add a call
to vector->setNull(vectorIndex, false) to reset the null indicator, mirroring
the other type branches (e.g., Boolean, Integer, Float, String) so the written
timestamp is treated as non-null.
Comment on lines
+117
to
+153
| case ColumnType::Array: { | ||
| auto arrayVector = std::dynamic_pointer_cast<ArrayVector>(vector); | ||
| std::string jsonString; | ||
| if (value->is<::clp::ffi::EightByteEncodedTextAst>()) { | ||
| auto decodeResult = | ||
| value->get_immutable_view<::clp::ffi::EightByteEncodedTextAst>() | ||
| .to_string(); | ||
| if (!decodeResult.has_value()) { | ||
| continue; | ||
| } | ||
| jsonString = std::move(decodeResult.value()); | ||
| } else { | ||
| auto decodeResult = | ||
| value->get_immutable_view<::clp::ffi::FourByteEncodedTextAst>() | ||
| .to_string(); | ||
| if (!decodeResult.has_value()) { | ||
| continue; | ||
| } | ||
| jsonString = std::move(decodeResult.value()); | ||
| } | ||
|
|
||
| size_t numElements{0ULL}; | ||
| auto elements = arrayVector->elements()->asFlatVector<StringView>(); | ||
| auto obj = arrayParser_.iterate(jsonString); | ||
| std::vector<std::string_view> rawElements; | ||
| for (auto arrayElement : obj.get_array()) { | ||
| auto raw_element = simdjson::to_json_string(arrayElement).value(); | ||
| rawElements.emplace_back(raw_element); | ||
| } | ||
| elements->resize(rawElements.size()); | ||
| for (auto& raw_element : rawElements) { | ||
| elements->set(numElements++, StringView(raw_element)); | ||
| } | ||
| arrayVector->setOffsetAndSize(vectorIndex, 0ULL, numElements); | ||
| arrayVector->setNull(vectorIndex, false); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd velox-connector && find . -name "ClpIrVectorLoader.cpp" -type fRepository: y-scope/clp-plugin-presto-connector
Length of output: 131
🏁 Script executed:
cat -n src/connector/search_lib/ir/ClpIrVectorLoader.cpp | head -200Repository: y-scope/clp-plugin-presto-connector
Length of output: 161
🏁 Script executed:
cat -n src/connector/search_lib/ir/ClpIrVectorLoader.cpp | sed -n '80,160p'Repository: y-scope/clp-plugin-presto-connector
Length of output: 161
🏁 Script executed:
find . -name "ClpIrVectorLoader.cpp" -type fRepository: y-scope/clp-plugin-presto-connector
Length of output: 147
🏁 Script executed:
git ls-files | grep -i "ClpIrVectorLoader"Repository: y-scope/clp-plugin-presto-connector
Length of output: 209
🏁 Script executed:
cat -n velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp | sed -n '80,160p'Repository: y-scope/clp-plugin-presto-connector
Length of output: 3636
🏁 Script executed:
wc -l velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cppRepository: y-scope/clp-plugin-presto-connector
Length of output: 149
🏁 Script executed:
cat -n velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp | sed -n '1,80p'Repository: y-scope/clp-plugin-presto-connector
Length of output: 3598
🏁 Script executed:
cat -n velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp | sed -n '24,50p'Repository: y-scope/clp-plugin-presto-connector
Length of output: 1177
🏁 Script executed:
rg "elementIndex" velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cppRepository: y-scope/clp-plugin-presto-connector
Length of output: 61
🏁 Script executed:
rg "elements->resize\|setOffsetAndSize" velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cppRepository: y-scope/clp-plugin-presto-connector
Length of output: 61
🏁 Script executed:
cat -n velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp | sed -n '117,153p'Repository: y-scope/clp-plugin-presto-connector
Length of output: 1832
🏁 Script executed:
rg "if \(!" velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp -A 1 -B 1Repository: y-scope/clp-plugin-presto-connector
Length of output: 559
Array handling corrupts multi-row results due to per-row vector resizing.
In the loop at line 30 iterating over multiple rows, the Array case (lines 117–153) resizes the shared elements vector to the current row's element count, destroying previous rows' data. Each row receives offset 0ULL, causing all rows to point to the same corrupted backing region. The elements vector must be grown incrementally across rows using a persistent element index, not reset and resized per iteration.
Additionally, lines 124 and 132 use if (!decodeResult.has_value()), which violates the code style guideline preferring false == <expression> over !<expression>.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp` around
lines 117 - 153, The Array branch currently resizes the shared elements flat
vector per-row and always sets each row's offset to 0, corrupting multi-row
results; change this to append elements to the shared elements buffer using a
persistent cumulative index (e.g., a local persistent size_t
elementsOffset/nextElementIndex) so each row sets
arrayVector->setOffsetAndSize(vectorIndex, elementsOffset, rowCount) and then
increments elementsOffset by rowCount (grow or reserve the elements flat vector
rather than resetting it), and ensure you use elements->set(elementsOffset + i,
...) for each appended element; also replace the style-violating checks `if
(!decodeResult.has_value())` with the preferred `if (false ==
decodeResult.has_value())` in both decode branches.
Comment on lines
+36
to
+39
| #if defined(_WIN32) | ||
| LPCH envStrings = GetEnvironmentStringsA(); | ||
| if (!envStrings) | ||
| return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Normalise these boolean checks to the project style.
The new test helper uses unary ! in a couple of places. Please rewrite them as nullptr == envStrings / false == expectedValue.has_value() to match the repo rule.
As per coding guidelines, **/*.{cpp,h,hpp,java,js,jsx,tpp,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
Also applies to: 167-171
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/connector/tests/ClpConfigTest.cpp` around lines 36 - 39,
Normalize the unary negation checks to the project's preferred style: replace
uses of "!envStrings" with "nullptr == envStrings" after
GetEnvironmentStringsA() and replace uses of "!expectedValue.has_value()" with
"false == expectedValue.has_value()" (or equivalent comparison) in the test
helper; update both the envStrings check (symbol: envStrings /
GetEnvironmentStringsA) and the has_value check (symbol:
expectedValue.has_value) around the other occurrence referenced (lines ~167-171)
so they conform to the coding guideline.
Comment on lines
+80
to
+83
| static std::string getExampleFilePath(const std::string& filePath) { | ||
| std::string current_path = fs::current_path().string(); | ||
| return current_path + "/examples/" + filePath; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
fs::current_path() makes test data discovery CWD-dependent.
getExampleFilePath resolves examples against the process's current working directory at runtime. If the test binary is launched from anywhere other than its own directory (e.g. from a build subdir or from CMake's ctest with a different WORKING_DIRECTORY), every test will fail to open its .clps/.clp.zst fixture. Prefer resolving the examples directory from a path passed in by the build system (e.g. a compile-time -D macro pointing at ${CMAKE_CURRENT_SOURCE_DIR}/examples) so the lookup is stable regardless of CWD.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 80-80: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 81-81: variable 'current_path' is not initialized
(cppcoreguidelines-init-variables)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/connector/tests/ClpConnectorTest.cpp` around lines 80 -
83, getExampleFilePath currently uses fs::current_path() making test fixture
lookup CWD-dependent; change it to resolve examples from a stable, build-time
path supplied by the build system (e.g. a compile-time macro like EXAMPLES_DIR
or a const string defined via -DEXAMPLES_DIR="...") instead of
fs::current_path(), and update getExampleFilePath to concatenate that
macro/constant with the relative filePath so tests find examples regardless of
working directory; ensure the macro is documented in CMake and provide a
sensible fallback or static path for local dev if desired.
Comment on lines
+35
to
+37
| bool operator<(const ColumnHandle& o) const override { | ||
| return columnName < dynamic_cast<const ClpColumnHandle&>(o).columnName; | ||
| } |
There was a problem hiding this comment.
Unsafe dynamic_cast without exception handling.
The dynamic_cast on Line 36 will throw std::bad_cast if the runtime type of o is not ClpColumnHandle. Since this is a virtual override that could be invoked polymorphically with any ColumnHandle subtype, a type mismatch would cause an uncaught exception and terminate the program.
Consider adding a type check or documenting the precondition that this operator should only be called with matching types.
🛡️ Proposed fix with safe cast
bool operator<(const ColumnHandle& o) const override {
+ auto* clpHandle = dynamic_cast<const ClpColumnHandle*>(&o);
+ VELOX_CHECK_NOT_NULL(clpHandle, "Cannot compare ClpColumnHandle with {}", o._type);
+ return columnName < clpHandle->columnName;
- return columnName < dynamic_cast<const ClpColumnHandle&>(o).columnName;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/protocol/presto_protocol_clp.h` around lines 35 - 37,
operator< currently uses dynamic_cast<const ClpColumnHandle&>(o) which will
throw std::bad_cast if o is not a ClpColumnHandle; replace the unsafe cast with
a safe RTTI check: use dynamic_cast<const ClpColumnHandle*>(&o) and if the
result is non-null compare columnName to that->columnName, otherwise decide a
deterministic fallback (e.g. compare std::type_index(typeid(*this)) vs
std::type_index(typeid(o)) or assert/log and return false) so the operator never
throws; update the implementation inside the ClpColumnHandle::operator< to
perform this null-check and use the chosen fallback behavior consistently.
20001020ycx
force-pushed
the
feat/2026-05-11-velox-connector-initialization
branch
from
May 12, 2026 14:38
42b4661 to
cf2fedf
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (16)
velox-connector/src/connector/ClpConnectorSplit.h (1)
24-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate split type before casting to
SplitType.Directly casting
inttoSplitTypeaccepts invalid ordinals and can drive wrong runtime behaviour (including misleadingtoString()output).Proposed fix
ClpConnectorSplit( const std::string& connectorId, const std::string& path, const int type, std::shared_ptr<std::string> kqlQuery) : connector::ConnectorSplit(connectorId), path_(path), - type_(static_cast<SplitType>(type)), + type_([&]() { + VELOX_CHECK( + type == static_cast<int>(SplitType::kArchive) || + type == static_cast<int>(SplitType::kIr), + "Invalid CLP split type: {} for path: {}", + type, + path); + return static_cast<SplitType>(type); + }()), kqlQuery_(std::move(kqlQuery)) {}Also applies to: 34-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/ClpConnectorSplit.h` around lines 24 - 32, The ClpConnectorSplit constructor currently casts the incoming int directly to SplitType (in the initializer setting type_) which allows invalid ordinals; update the constructor(s) (ClpConnectorSplit(...)) to validate the int against the valid SplitType range/values before assigning to type_ (e.g., check min/max enum values or use a helper like SplitTypeFromInt that returns optional/throws), and handle invalid values by defaulting to a safe SplitType or throwing/logging an error; apply the same validation change to the other overloaded constructor(s) in the file (the one at lines 34-39) so no raw static_cast<int> -> SplitType occurs without a bounds check.velox-connector/src/connector/ClpConfig.h (1)
17-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
<memory>to keep the header self-contained.
std::shared_ptris part of the public interface here, so omitting<memory>makes inclusion brittle.Proposed fix
`#pragma` once + +#include <memory>Also applies to: 41-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/ClpConfig.h` around lines 17 - 19, This header is missing the <memory> include even though it exposes std::shared_ptr in its public interface; make the header self-contained by adding `#include` <memory> near the top (before the namespace facebook::velox::config) so references to std::shared_ptr used by ClpConfig and any related declarations in this header are resolved without relying on transitive includes.velox-connector/src/connector/search_lib/ClpS3AuthProviderBase.cpp (2)
48-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against null pointer dereference in sanity check.
Line 50 calls
std::strcmp(valueCStr, valueForCheck)without first verifying thatvalueForCheckis non-null. Ifgetenvunexpectedly returnsnullptr(due to environment corruption or a race condition),strcmpwill crash instead of providing a clear assertion failure.🛡️ Proposed fix
// Sanity check auto valueForCheck = std::getenv(keyCStr); + VELOX_CHECK_NOT_NULL(valueForCheck, "Failed to read back environment variable: {}", keyStr); VELOX_CHECK_EQ(0, std::strcmp(valueCStr, valueForCheck));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/search_lib/ClpS3AuthProviderBase.cpp` around lines 48 - 51, The sanity check in ClpS3AuthProviderBase.cpp calls std::strcmp(valueCStr, valueForCheck) without ensuring valueForCheck is non-null; modify the check to first assert valueForCheck is not nullptr (e.g., VELOX_CHECK_NE(valueForCheck, nullptr)) before calling std::strcmp, then perform the existing equality assertion comparing valueCStr and valueForCheck; reference variables: valueForCheck, valueCStr, keyCStr in the surrounding sanity-check logic.
57-66:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Undefined behaviour from passing temporary to
_putenv.On line 59,
fmt::format("{}=", keyCStr)creates a temporarystd::string, which is then implicitly converted toconst char*and passed to_putenv. According to MSDN documentation,_putenvdoes not copy the string—it stores the pointer directly in the environment. When the temporary is destroyed at the end of the statement, the environment contains a dangling pointer, causing undefined behaviour.For consistency with line 39 and to fix the bug, use
_putenv_s(keyCStr, "")which properly handles the lifetime.🐛 Proposed fix
`#ifdef` _WIN32 // Windows version - err = _putenv(fmt::format("{}=", keyCStr)); + err = _putenv_s(keyCStr, ""); `#elif` defined(__unix__) || defined(__APPLE__)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/search_lib/ClpS3AuthProviderBase.cpp` around lines 57 - 66, The Windows branch currently calls _putenv(fmt::format("{}=", keyCStr)) which passes a pointer to a temporary string and leads to undefined behaviour; replace that call with _putenv_s(keyCStr, "") so the runtime stores the value safely (mirror the lifetime-safe approach used elsewhere), keep the Unix/macOS branch using unsetenv(keyCStr), and ensure the existing VELOX_CHECK_EQ(0, err) validation remains after the call to check for errors.velox-connector/src/connector/CMakeLists.txt (1)
10-12: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winConsider making C++20 a PUBLIC requirement.
Since
presto_clp_connectoris a STATIC library with PUBLIC include directories (lines 14-19), any C++20 language features used in its public headers would require consuming targets to also compile with C++20. UsingPRIVATEhere means only the library's.cppfiles get C++20, potentially causing compilation failures for consumers.🔧 Suggested fix
target_compile_features(presto_clp_connector - PRIVATE cxx_std_20 + PUBLIC cxx_std_20 )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/CMakeLists.txt` around lines 10 - 12, The target_compile_features call for the target presto_clp_connector currently marks cxx_std_20 as PRIVATE; change it to PUBLIC so that consuming targets that include the connector's public headers also compile with C++20. Update the target_compile_features(presto_clp_connector PRIVATE cxx_std_20) invocation to use PUBLIC so the language requirement propagates to dependents (affecting the target_compile_features setting for presto_clp_connector).velox-connector/src/connector/ClpDataSource.cpp (4)
127-132: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the coding guideline
false == <expression>style.Replace
!pushDownQuery->empty()withfalse == pushDownQuery->empty()to align with the repository convention.♻️ Proposed fix
auto pushDownQuery = clpSplit->kqlQuery_; - if (pushDownQuery && !pushDownQuery->empty()) { + if (pushDownQuery && false == pushDownQuery->empty()) { cursor_->executeQuery(*pushDownQuery, fields_);As per coding guidelines,
**/*.{cpp,h,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/ClpDataSource.cpp` around lines 127 - 132, The condition uses the negation operator; update the check to follow the repository style by replacing the `!pushDownQuery->empty()` expression with the equivalent `false == pushDownQuery->empty()` in the block that handles `clpSplit->kqlQuery_` (where `pushDownQuery` is used) so that the `if` becomes `if (pushDownQuery && false == pushDownQuery->empty())` before calling `cursor_->executeQuery(*pushDownQuery, fields_)`.
102-105:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing null check after
dynamic_pointer_cast<ClpConnectorSplit>causes NPE on type mismatch.If a
ConnectorSplitof a different runtime type reaches this method,clpSplitbecomes null and line 105 (clpSplit->path_) dereferences nullptr. Validate the cast immediately, mirroring theVELOX_CHECK_NOT_NULLpattern used for column handles in the constructor.🛡️ Proposed fix
void ClpDataSource::addSplit(std::shared_ptr<ConnectorSplit> split) { auto clpSplit = std::dynamic_pointer_cast<ClpConnectorSplit>(split); + VELOX_CHECK_NOT_NULL( + clpSplit, "Split must be an instance of ClpConnectorSplit"); std::string splitPath = clpSplit->path_;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/ClpDataSource.cpp` around lines 102 - 105, Add a null-check after the dynamic cast in ClpDataSource::addSplit to avoid dereferencing clpSplit when the runtime type is not ClpConnectorSplit: validate that std::dynamic_pointer_cast<ClpConnectorSplit>(split) returned non-null (using the same VELOX_CHECK_NOT_NULL pattern used elsewhere) and log or throw a clear error if it is null before accessing clpSplit->path_; this ensures ConnectorSplit type mismatches are caught early and prevents NPEs.
135-146:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftZero-filtered-row condition prematurely terminates IR splits and double-counts rows.
Returning
nullptrwhenrowsFiltered == 0(line 140) incorrectly signals EOF for IR splits. UnlikeClpArchiveCursor, which loops internally until rows are found or the stream is exhausted,ClpIrCursor::deserialize()processes one batch per call and clearsfilteredLogEvents_before each batch. A batch with zero filtered rows does not mean EOF—more unread events may remain in the stream.Additionally,
ClpIrCursor::fetchNext()returns the cumulativeirDeserializer_->get_num_log_events_deserialized()count rather than a per-batch delta. As a result, line 143 (completedRows_ += rowsScanned) double-counts rows on successive IR batches, accumulating the total deserialized count repeatedly.Recommend: (1) Check stream completion (e.g.,
irDeserializer_->is_stream_completed()) before returningnullptr, and (2) modifyClpIrCursor::fetchNext()to return only the incremental batch count (cache the previous cumulative count and return the delta).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/ClpDataSource.cpp` around lines 135 - 146, The current ClpDataSource::next implementation mistakenly returns nullptr when rowsFiltered == 0 (treating zero-filtered batch as EOF) and double-counts IR rows because cursor_->fetchNext() returns a cumulative count; change ClpDataSource::next to only return nullptr when the underlying IR stream is truly completed (use irDeserializer_->is_stream_completed() via ClpIrCursor) and otherwise continue looping/returning an empty batch without signaling EOF; also stop adding the cumulative value to completedRows_ by making ClpIrCursor::fetchNext() return the per-batch delta (cache the previous irDeserializer_->get_num_log_events_deserialized() and return current - previous) and update completedRows_ with that delta instead of the cumulative rowsScanned.
37-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDead
clpTableHandlecast—add validation or remove.The
dynamic_pointer_cast<const ClpTableHandle>result is never used or validated. Either addVELOX_CHECK_NOT_NULL(clpTableHandle, ...)to reject incompatible table handles (mirroring the column-handle checks at lines 50-53), or remove the unused cast entirely.🛡️ Proposed fix
auto clpTableHandle = std::dynamic_pointer_cast<const ClpTableHandle>(tableHandle); + VELOX_CHECK_NOT_NULL( + clpTableHandle, + "TableHandle must be an instance of ClpTableHandle"); storageType_ = clpConfig->storageType();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/ClpDataSource.cpp` around lines 37 - 40, The dynamic_pointer_cast to const ClpTableHandle produces clpTableHandle which is never used or validated; add a null-check similar to the column-handle checks by invoking VELOX_CHECK_NOT_NULL(clpTableHandle, "Expected ClpTableHandle for tableHandle") immediately after the cast to reject incompatible table handles (or alternatively remove the unused clpTableHandle variable and cast if you prefer); ensure the check uses the same error style as the existing column-handle validation so failures are handled consistently.velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp (4)
32-32: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the coding guideline
false == <expression>style.Replace
!isResolved_withfalse == isResolved_to align with the repository convention.As per coding guidelines,
**/*.{cpp,h,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp` at line 32, The conditional in ClpIrVectorLoader.cpp uses negation operator (!isResolved_) which violates the repository style; change the condition to use the guideline form (false == isResolved_) in the relevant if statement inside the ClpIrVectorLoader code (refer to the isResolved_ member and the if block where it is checked) so the check reads false == isResolved_ instead of !isResolved_.
102-115:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing null-bit reset for Timestamp values.
The Timestamp case branch successfully sets timestamp values but never clears the null bit (unlike Boolean, Integer, Float, and String branches that all call
vector->setNull(vectorIndex, false)). Converted timestamp values remain marked NULL despite being set, corrupting query results.🐛 Proposed fix
case ColumnType::Timestamp: { auto timestampVector = vector->asFlatVector<Timestamp>(); if (value->is<double>()) { timestampVector->set( vectorIndex, convertToVeloxTimestamp(value->get_immutable_view<double>())); } else if (value->is<int64_t>()) { timestampVector->set( vectorIndex, convertToVeloxTimestamp(value->get_immutable_view<int64_t>())); } else { VELOX_FAIL("Unsupported timestamp type"); } + vector->setNull(vectorIndex, false); break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp` around lines 102 - 115, The Timestamp branch in ClpIrVectorLoader.cpp (inside the switch on ColumnType::Timestamp) sets values on timestampVector using convertToVeloxTimestamp but never clears the null bit, leaving entries marked NULL; after the timestampVector->set(...) calls (both double and int64_t paths) add a call to vector->setNull(vectorIndex, false) so the null bitmap is updated, ensuring Timestamp values are not left as NULL.
117-153:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftArray handling corrupts multi-row results.
The Array case processes multiple rows in a loop (line 30) but resizes the shared
elementsvector per-row (line 146) and always sets each row's offset to0ULL(line 150), destroying all previous rows' array data. Each row must append elements to a persistent backing buffer using a cumulative element index.🐛 Proposed fix
Track a persistent element offset outside the row loop and append elements incrementally:
+ // Outside the row loop (e.g., member variable or local before loop) + size_t elementsOffset = 0; + for (int vectorIndex : rows) { ... case ColumnType::Array: { auto arrayVector = std::dynamic_pointer_cast<ArrayVector>(vector); std::string jsonString; if (value->is<::clp::ffi::EightByteEncodedTextAst>()) { auto decodeResult = value->get_immutable_view<::clp::ffi::EightByteEncodedTextAst>() .to_string(); - if (!decodeResult.has_value()) { + if (false == decodeResult.has_value()) { continue; } jsonString = std::move(decodeResult.value()); } else { auto decodeResult = value->get_immutable_view<::clp::ffi::FourByteEncodedTextAst>() .to_string(); - if (!decodeResult.has_value()) { + if (false == decodeResult.has_value()) { continue; } jsonString = std::move(decodeResult.value()); } - size_t numElements{0ULL}; auto elements = arrayVector->elements()->asFlatVector<StringView>(); auto obj = arrayParser_.iterate(jsonString); std::vector<std::string_view> rawElements; for (auto arrayElement : obj.get_array()) { auto raw_element = simdjson::to_json_string(arrayElement).value(); rawElements.emplace_back(raw_element); } - elements->resize(rawElements.size()); + size_t rowElementCount = rawElements.size(); + elements->resize(elementsOffset + rowElementCount); + size_t i = 0; for (auto& raw_element : rawElements) { - elements->set(numElements++, StringView(raw_element)); + elements->set(elementsOffset + i++, StringView(raw_element)); } - arrayVector->setOffsetAndSize(vectorIndex, 0ULL, numElements); + arrayVector->setOffsetAndSize(vectorIndex, elementsOffset, rowElementCount); arrayVector->setNull(vectorIndex, false); + elementsOffset += rowElementCount; break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp` around lines 117 - 153, The Array case in ClpIrVectorLoader.cpp currently resizes the per-row elements flat vector and sets each row's offset to 0 (symbols: arrayVector, elements, arrayParser_.iterate, rawElements, numElements, setOffsetAndSize), which corrupts multi-row results; fix it by making the elements backing buffer persistent across rows (move the cumulative element index/offset out of the per-row block), append each row's parsed items to elements without calling resize per row, use the cumulative offset when calling arrayVector->setOffsetAndSize(vectorIndex, cumulativeOffset, rowCount) and advance the cumulative offset by rowCount, and ensure setNull is set correctly per-row instead of overwriting previous data.
62-73: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the coding guideline
false == <expression>style in decode branches.Replace
!decodeResult.has_value()withfalse == decodeResult.has_value()in both the EightByteEncodedTextAst and FourByteEncodedTextAst decode branches to align with the repository convention.As per coding guidelines,
**/*.{cpp,h,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.Also applies to: 124-135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp` around lines 62 - 73, The checks using negation on optional results should follow the repository convention: replace instances of "!decodeResult.has_value()" with "false == decodeResult.has_value()" in the decode branches for ::clp::ffi::EightByteEncodedTextAst and ::clp::ffi::FourByteEncodedTextAst (the blocks that call .to_string() and then use stringVector->set(vectorIndex, StringView(decodeResult.value()))); also update the same pattern where decodeResult is used elsewhere in the file (the other decode branch using to_string()) to maintain consistent style.velox-connector/src/connector/search_lib/ir/ClpIrCursor.cpp (2)
90-90: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
false == ...for these negation checks.These two conditions still use
!, which violates the C++ guideline used in this repo.Proposed fix
- if (!queryHandlerResult) { + if (false == queryHandlerResult) { VLOG(2) << "Failed to create query handler for deserialization."; return ErrorCode::InternalError; }bool isResolved = - it != projectedColumnIdxNodeIdsMap_.end() && !it->second.empty(); + it != projectedColumnIdxNodeIdsMap_.end() && + false == it->second.empty();As per coding guidelines, "Prefer
false == <expression>rather than!<expression>."Also applies to: 230-231
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/search_lib/ir/ClpIrCursor.cpp` at line 90, Replace negation checks that use the '!' operator with the repository's preferred style `false == <expression>`; specifically change the condition `if (!queryHandlerResult)` to `if (false == queryHandlerResult)` and make the analogous replacements for the two other negation checks around lines 230–231 in ClpIrCursor.cpp (use the same `false == <expression>` pattern for those variables/expressions). Ensure parentheses are preserved exactly as in the original conditions and rebuild to validate.
102-110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop the dead null check and catch
open()failures.
std::make_sharedwill not returnnullptr, so Lines 105-109 are dead code. The actual failure point isirReaderZstdWrapper_->open(...), and right now an exception there escapesloadSplit()instead of being converted intoErrorCode::InternalError.Proposed fix
irReaderZstdWrapper_ = std::make_shared<::clp::streaming_compression::zstd::Decompressor>(); constexpr size_t cReaderBufferSize{64L * 1024L}; - if (nullptr == irReaderZstdWrapper_) { - VLOG(2) << "Failed to open kv-ir stream \"" << splitPath_ - << "\" for reading."; - return ErrorCode::InternalError; - } - irReaderZstdWrapper_->open(*irReader_, cReaderBufferSize); + try { + irReaderZstdWrapper_->open(*irReader_, cReaderBufferSize); + } catch (const std::exception& e) { + VLOG(2) << "Failed to open kv-ir stream \"" << splitPath_ + << "\" for reading: " << e.what(); + return ErrorCode::InternalError; + }Please verify the current
::clp::streaming_compression::zstd::Decompressor::openfailure contract in this branch before merging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/search_lib/ir/ClpIrCursor.cpp` around lines 102 - 110, Remove the dead nullptr check for irReaderZstdWrapper_ (std::make_shared never returns nullptr) and instead wrap the call to ::clp::streaming_compression::zstd::Decompressor::open(...) in a try/catch inside loadSplit(); on any exception or failure from open() catch it, log a descriptive message including splitPath_ and the exception/error details, and return ErrorCode::InternalError so open failures are converted to the proper error code rather than escaping.velox-connector/src/connector/search_lib/archive/ClpArchiveVectorLoader.cpp (1)
89-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the null bit after writing timestamp values.
This loop writes timestamp payloads but never marks the row non-null. When Velox reuses the result vector, rows that were null in a previous batch can stay null even after
vector->set(...)succeeds.Proposed fix
for (int vectorIndex : rows) { auto messageIndex = filteredRowIndices_->at(vectorIndex); if (clp_s::NodeType::Timestamp == Type) { auto reader{static_cast<clp_s::TimestampColumnReader*>(columnReader_)}; vector->set( vectorIndex, convertNanosecondEpochToVeloxTimestamp( reader->get_encoded_time(messageIndex))); } else if (clp_s::NodeType::Float == Type) { auto reader = static_cast<clp_s::FloatColumnReader*>(columnReader_); vector->set( vectorIndex, convertToVeloxTimestamp( std::get<double>(reader->extract_value(messageIndex)))); } else if (clp_s::NodeType::FormattedFloat == Type) { auto reader = static_cast<clp_s::FormattedFloatColumnReader*>(columnReader_); vector->set( vectorIndex, convertToVeloxTimestamp( std::get<double>(reader->extract_value(messageIndex)))); } else if (clp_s::NodeType::DictionaryFloat == Type) { auto reader = static_cast<clp_s::DictionaryFloatColumnReader*>(columnReader_); vector->set( vectorIndex, convertToVeloxTimestamp( std::get<double>(reader->extract_value(messageIndex)))); } else if (clp_s::NodeType::Integer == Type) { auto reader = static_cast<clp_s::Int64ColumnReader*>(columnReader_); vector->set( vectorIndex, convertToVeloxTimestamp( std::get<int64_t>(reader->extract_value(messageIndex)))); } else { auto reader = static_cast<clp_s::DeprecatedDateStringColumnReader*>(columnReader_); vector->set( vectorIndex, convertToVeloxTimestamp(reader->get_encoded_time(messageIndex))); } + vector->setNull(vectorIndex, false); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/connector/search_lib/archive/ClpArchiveVectorLoader.cpp` around lines 89 - 131, The loop writes timestamp values into the result vector but never clears the null bit, so previously-null rows can remain null; after every vector->set(vectorIndex, ...) call in ClpArchiveVectorLoader::[this loop] (the branches that call convertNanosecondEpochToVeloxTimestamp or convertToVeloxTimestamp and use readers like TimestampColumnReader, FloatColumnReader, FormattedFloatColumnReader, DictionaryFloatColumnReader, Int64ColumnReader, DeprecatedDateStringColumnReader), explicitly clear the null flag for that slot (e.g. call the FlatVector/Vector API to mark the index non-null such as vector->setNull(vectorIndex, false) or the equivalent null-bit clearing method) immediately after each vector->set(...) so the slot is marked non-null when a value is written.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@velox-connector/src/ClpPrestoToVeloxConnector.cpp`:
- Around line 31-33: The cast and subsequent error message access
connectorSplit->_type without first ensuring connectorSplit (and other handles
like column and tableHandle.connectorTableLayout) are non-null; change the
checks around the dynamic_cast of connectorSplit (the clpSplit variable) so you
validate connectorSplit is not null before referencing connectorSplit->_type in
the VELOX_CHECK_NOT_NULL call, and similarly guard any uses of column and
tableHandle.connectorTableLayout in the other occurrences (the checks around
lines 45-47 and 59-65) so the error path does not dereference ->_type on a null
pointer; update the VELOX_CHECK_NOT_NULL calls or add explicit null checks to
include safe, non-dereferencing messages when the incoming handle is null.
In `@velox-connector/src/connector/search_lib/ClpTimestampsUtils.h`:
- Around line 47-52: The estimatePrecision(T timestamp) function can UB by
negating INT64_MIN; update estimatePrecision (and callers like
convertToVeloxTimestamp) to handle the extremal case explicitly instead of doing
timestamp >= 0 ? timestamp : -timestamp. Detect timestamp ==
std::numeric_limits<int64_t>::min() (or use an unsigned absolute conversion) and
compute absTimestamp via a safe uint64_t path (or return the correct precision
for that value) before comparing against
kEpochMilliseconds1971/kEpochMicroseconds1971/kEpochNanoseconds1971 so no signed
overflow occurs.
In `@velox-connector/src/connector/search_lib/CMakeLists.txt`:
- Around line 10-12: The target_compile_features call for target clp_search
currently uses PRIVATE cxx_std_20 which only applies the C++20 requirement to
the library's implementation; change it to PUBLIC so consuming targets that
include clp_search's public headers also compile with C++20. Update the
invocation of target_compile_features(clp_search ...) to use PUBLIC cxx_std_20
to propagate the language requirement to dependents.
In `@velox-connector/src/protocol/CMakeLists.txt`:
- Around line 6-8: The target_compile_features call currently marks C++20 as
PRIVATE for the target named presto_clp_protocol; change it so C++20 is a PUBLIC
requirement (i.e., make the feature requirement public) so consumers compiling
against presto_clp_protocol also get cxx_std_20; update the
target_compile_features(presto_clp_protocol ...) invocation accordingly to use
the PUBLIC specifier so public headers that use C++20 compile correctly for
dependent targets.
In `@velox-connector/src/protocol/PrestoProtocolClp.h`:
- Around line 16-20: The header PrestoProtocolClp.h uses std::shared_ptr in
several struct members (the shared_ptr occurrences around the members reported
at lines 43, 57, 58) but doesn't include <memory>; add `#include` <memory> to the
top of PrestoProtocolClp.h alongside the other includes so the declarations that
reference std::shared_ptr compile without relying on transitive includes.
---
Duplicate comments:
In `@velox-connector/src/connector/ClpConfig.h`:
- Around line 17-19: This header is missing the <memory> include even though it
exposes std::shared_ptr in its public interface; make the header self-contained
by adding `#include` <memory> near the top (before the namespace
facebook::velox::config) so references to std::shared_ptr used by ClpConfig and
any related declarations in this header are resolved without relying on
transitive includes.
In `@velox-connector/src/connector/ClpConnectorSplit.h`:
- Around line 24-32: The ClpConnectorSplit constructor currently casts the
incoming int directly to SplitType (in the initializer setting type_) which
allows invalid ordinals; update the constructor(s) (ClpConnectorSplit(...)) to
validate the int against the valid SplitType range/values before assigning to
type_ (e.g., check min/max enum values or use a helper like SplitTypeFromInt
that returns optional/throws), and handle invalid values by defaulting to a safe
SplitType or throwing/logging an error; apply the same validation change to the
other overloaded constructor(s) in the file (the one at lines 34-39) so no raw
static_cast<int> -> SplitType occurs without a bounds check.
In `@velox-connector/src/connector/ClpDataSource.cpp`:
- Around line 127-132: The condition uses the negation operator; update the
check to follow the repository style by replacing the `!pushDownQuery->empty()`
expression with the equivalent `false == pushDownQuery->empty()` in the block
that handles `clpSplit->kqlQuery_` (where `pushDownQuery` is used) so that the
`if` becomes `if (pushDownQuery && false == pushDownQuery->empty())` before
calling `cursor_->executeQuery(*pushDownQuery, fields_)`.
- Around line 102-105: Add a null-check after the dynamic cast in
ClpDataSource::addSplit to avoid dereferencing clpSplit when the runtime type is
not ClpConnectorSplit: validate that
std::dynamic_pointer_cast<ClpConnectorSplit>(split) returned non-null (using the
same VELOX_CHECK_NOT_NULL pattern used elsewhere) and log or throw a clear error
if it is null before accessing clpSplit->path_; this ensures ConnectorSplit type
mismatches are caught early and prevents NPEs.
- Around line 135-146: The current ClpDataSource::next implementation mistakenly
returns nullptr when rowsFiltered == 0 (treating zero-filtered batch as EOF) and
double-counts IR rows because cursor_->fetchNext() returns a cumulative count;
change ClpDataSource::next to only return nullptr when the underlying IR stream
is truly completed (use irDeserializer_->is_stream_completed() via ClpIrCursor)
and otherwise continue looping/returning an empty batch without signaling EOF;
also stop adding the cumulative value to completedRows_ by making
ClpIrCursor::fetchNext() return the per-batch delta (cache the previous
irDeserializer_->get_num_log_events_deserialized() and return current -
previous) and update completedRows_ with that delta instead of the cumulative
rowsScanned.
- Around line 37-40: The dynamic_pointer_cast to const ClpTableHandle produces
clpTableHandle which is never used or validated; add a null-check similar to the
column-handle checks by invoking VELOX_CHECK_NOT_NULL(clpTableHandle, "Expected
ClpTableHandle for tableHandle") immediately after the cast to reject
incompatible table handles (or alternatively remove the unused clpTableHandle
variable and cast if you prefer); ensure the check uses the same error style as
the existing column-handle validation so failures are handled consistently.
In `@velox-connector/src/connector/CMakeLists.txt`:
- Around line 10-12: The target_compile_features call for the target
presto_clp_connector currently marks cxx_std_20 as PRIVATE; change it to PUBLIC
so that consuming targets that include the connector's public headers also
compile with C++20. Update the target_compile_features(presto_clp_connector
PRIVATE cxx_std_20) invocation to use PUBLIC so the language requirement
propagates to dependents (affecting the target_compile_features setting for
presto_clp_connector).
In `@velox-connector/src/connector/search_lib/archive/ClpArchiveVectorLoader.cpp`:
- Around line 89-131: The loop writes timestamp values into the result vector
but never clears the null bit, so previously-null rows can remain null; after
every vector->set(vectorIndex, ...) call in ClpArchiveVectorLoader::[this loop]
(the branches that call convertNanosecondEpochToVeloxTimestamp or
convertToVeloxTimestamp and use readers like TimestampColumnReader,
FloatColumnReader, FormattedFloatColumnReader, DictionaryFloatColumnReader,
Int64ColumnReader, DeprecatedDateStringColumnReader), explicitly clear the null
flag for that slot (e.g. call the FlatVector/Vector API to mark the index
non-null such as vector->setNull(vectorIndex, false) or the equivalent null-bit
clearing method) immediately after each vector->set(...) so the slot is marked
non-null when a value is written.
In `@velox-connector/src/connector/search_lib/ClpS3AuthProviderBase.cpp`:
- Around line 48-51: The sanity check in ClpS3AuthProviderBase.cpp calls
std::strcmp(valueCStr, valueForCheck) without ensuring valueForCheck is
non-null; modify the check to first assert valueForCheck is not nullptr (e.g.,
VELOX_CHECK_NE(valueForCheck, nullptr)) before calling std::strcmp, then perform
the existing equality assertion comparing valueCStr and valueForCheck; reference
variables: valueForCheck, valueCStr, keyCStr in the surrounding sanity-check
logic.
- Around line 57-66: The Windows branch currently calls
_putenv(fmt::format("{}=", keyCStr)) which passes a pointer to a temporary
string and leads to undefined behaviour; replace that call with
_putenv_s(keyCStr, "") so the runtime stores the value safely (mirror the
lifetime-safe approach used elsewhere), keep the Unix/macOS branch using
unsetenv(keyCStr), and ensure the existing VELOX_CHECK_EQ(0, err) validation
remains after the call to check for errors.
In `@velox-connector/src/connector/search_lib/ir/ClpIrCursor.cpp`:
- Line 90: Replace negation checks that use the '!' operator with the
repository's preferred style `false == <expression>`; specifically change the
condition `if (!queryHandlerResult)` to `if (false == queryHandlerResult)` and
make the analogous replacements for the two other negation checks around lines
230–231 in ClpIrCursor.cpp (use the same `false == <expression>` pattern for
those variables/expressions). Ensure parentheses are preserved exactly as in the
original conditions and rebuild to validate.
- Around line 102-110: Remove the dead nullptr check for irReaderZstdWrapper_
(std::make_shared never returns nullptr) and instead wrap the call to
::clp::streaming_compression::zstd::Decompressor::open(...) in a try/catch
inside loadSplit(); on any exception or failure from open() catch it, log a
descriptive message including splitPath_ and the exception/error details, and
return ErrorCode::InternalError so open failures are converted to the proper
error code rather than escaping.
In `@velox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cpp`:
- Line 32: The conditional in ClpIrVectorLoader.cpp uses negation operator
(!isResolved_) which violates the repository style; change the condition to use
the guideline form (false == isResolved_) in the relevant if statement inside
the ClpIrVectorLoader code (refer to the isResolved_ member and the if block
where it is checked) so the check reads false == isResolved_ instead of
!isResolved_.
- Around line 102-115: The Timestamp branch in ClpIrVectorLoader.cpp (inside the
switch on ColumnType::Timestamp) sets values on timestampVector using
convertToVeloxTimestamp but never clears the null bit, leaving entries marked
NULL; after the timestampVector->set(...) calls (both double and int64_t paths)
add a call to vector->setNull(vectorIndex, false) so the null bitmap is updated,
ensuring Timestamp values are not left as NULL.
- Around line 117-153: The Array case in ClpIrVectorLoader.cpp currently resizes
the per-row elements flat vector and sets each row's offset to 0 (symbols:
arrayVector, elements, arrayParser_.iterate, rawElements, numElements,
setOffsetAndSize), which corrupts multi-row results; fix it by making the
elements backing buffer persistent across rows (move the cumulative element
index/offset out of the per-row block), append each row's parsed items to
elements without calling resize per row, use the cumulative offset when calling
arrayVector->setOffsetAndSize(vectorIndex, cumulativeOffset, rowCount) and
advance the cumulative offset by rowCount, and ensure setNull is set correctly
per-row instead of overwriting previous data.
- Around line 62-73: The checks using negation on optional results should follow
the repository convention: replace instances of "!decodeResult.has_value()" with
"false == decodeResult.has_value()" in the decode branches for
::clp::ffi::EightByteEncodedTextAst and ::clp::ffi::FourByteEncodedTextAst (the
blocks that call .to_string() and then use stringVector->set(vectorIndex,
StringView(decodeResult.value()))); also update the same pattern where
decodeResult is used elsewhere in the file (the other decode branch using
to_string()) to maintain consistent style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c2f133e-58b9-46ce-85d8-14b72e6c0351
⛔ Files ignored due to path filters (4)
velox-connector/plugin.mapis excluded by!**/*.mapvelox-connector/src/connector/tests/examples/test_1_ir.clp.zstis excluded by!**/*.zstvelox-connector/src/connector/tests/examples/test_2_ir.clp.zstis excluded by!**/*.zstvelox-connector/src/connector/tests/examples/test_4_ir.clp.zstis excluded by!**/*.zst
📒 Files selected for processing (59)
velox-connector/.gitignorevelox-connector/CMakeLists.txtvelox-connector/src/CMakeLists.txtvelox-connector/src/ClpPluginEntry.cppvelox-connector/src/ClpPrestoToVeloxConnector.cppvelox-connector/src/ClpPrestoToVeloxConnector.hvelox-connector/src/connector/CMakeLists.txtvelox-connector/src/connector/ClpColumnHandle.hvelox-connector/src/connector/ClpConfig.cppvelox-connector/src/connector/ClpConfig.hvelox-connector/src/connector/ClpConnector.cppvelox-connector/src/connector/ClpConnector.hvelox-connector/src/connector/ClpConnectorSplit.hvelox-connector/src/connector/ClpDataSource.cppvelox-connector/src/connector/ClpDataSource.hvelox-connector/src/connector/ClpTableHandle.cppvelox-connector/src/connector/ClpTableHandle.hvelox-connector/src/connector/search_lib/BaseClpCursor.cppvelox-connector/src/connector/search_lib/BaseClpCursor.hvelox-connector/src/connector/search_lib/CMakeLists.txtvelox-connector/src/connector/search_lib/ClpPackageS3AuthProvider.cppvelox-connector/src/connector/search_lib/ClpPackageS3AuthProvider.hvelox-connector/src/connector/search_lib/ClpS3AuthProviderBase.cppvelox-connector/src/connector/search_lib/ClpS3AuthProviderBase.hvelox-connector/src/connector/search_lib/ClpTimestampsUtils.hvelox-connector/src/connector/search_lib/archive/CMakeLists.txtvelox-connector/src/connector/search_lib/archive/ClpArchiveCursor.cppvelox-connector/src/connector/search_lib/archive/ClpArchiveCursor.hvelox-connector/src/connector/search_lib/archive/ClpArchiveJsonStringVectorLoader.cppvelox-connector/src/connector/search_lib/archive/ClpArchiveJsonStringVectorLoader.hvelox-connector/src/connector/search_lib/archive/ClpArchiveVectorLoader.cppvelox-connector/src/connector/search_lib/archive/ClpArchiveVectorLoader.hvelox-connector/src/connector/search_lib/archive/ClpQueryRunner.cppvelox-connector/src/connector/search_lib/archive/ClpQueryRunner.hvelox-connector/src/connector/search_lib/ir/CMakeLists.txtvelox-connector/src/connector/search_lib/ir/ClpIrCursor.cppvelox-connector/src/connector/search_lib/ir/ClpIrCursor.hvelox-connector/src/connector/search_lib/ir/ClpIrJsonStringVectorLoader.cppvelox-connector/src/connector/search_lib/ir/ClpIrJsonStringVectorLoader.hvelox-connector/src/connector/search_lib/ir/ClpIrUnitHandler.hvelox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.cppvelox-connector/src/connector/search_lib/ir/ClpIrVectorLoader.hvelox-connector/src/connector/tests/CMakeLists.txtvelox-connector/src/connector/tests/ClpConfigTest.cppvelox-connector/src/connector/tests/ClpConnectorTest.cppvelox-connector/src/connector/tests/examples/test_1.clpsvelox-connector/src/connector/tests/examples/test_1.ndjsonvelox-connector/src/connector/tests/examples/test_2.clpsvelox-connector/src/connector/tests/examples/test_2.ndjsonvelox-connector/src/connector/tests/examples/test_3.clpsvelox-connector/src/connector/tests/examples/test_3.ndjsonvelox-connector/src/connector/tests/examples/test_4.ndjsonvelox-connector/src/connector/tests/examples/test_5.clpsvelox-connector/src/connector/tests/examples/test_5.ndjsonvelox-connector/src/connector/tests/examples/test_5.v0.5.0.clpsvelox-connector/src/protocol/CMakeLists.txtvelox-connector/src/protocol/ClpConnectorProtocol.cppvelox-connector/src/protocol/ClpConnectorProtocol.hvelox-connector/src/protocol/PrestoProtocolClp.h
Comment on lines
+31
to
+33
| auto clpSplit = dynamic_cast<const protocol::clp::ClpSplit*>(connectorSplit); | ||
| VELOX_CHECK_NOT_NULL( | ||
| clpSplit, "Unexpected split type {}", connectorSplit->_type); |
There was a problem hiding this comment.
Guard the incoming handles before using _type in the error path.
If connectorSplit, column, or tableHandle.connectorTableLayout is null, these checks will dereference ->_type while already handling the failed cast, so the worker crashes before surfacing the intended validation error.
Proposed fix
+ VELOX_CHECK_NOT_NULL(connectorSplit, "Connector split must not be null");
auto clpSplit = dynamic_cast<const protocol::clp::ClpSplit*>(connectorSplit);
VELOX_CHECK_NOT_NULL(
clpSplit, "Unexpected split type {}", connectorSplit->_type);+ VELOX_CHECK_NOT_NULL(column, "Column handle must not be null");
auto clpColumn = dynamic_cast<const protocol::clp::ClpColumnHandle*>(column);
VELOX_CHECK_NOT_NULL(
clpColumn, "Unexpected column handle type {}", column->_type);+ VELOX_CHECK_NOT_NULL(
+ tableHandle.connectorTableLayout,
+ "Connector table layout must not be null");
auto clpLayout =
std::dynamic_pointer_cast<const protocol::clp::ClpTableLayoutHandle>(
tableHandle.connectorTableLayout);
VELOX_CHECK_NOT_NULL(
clpLayout,
"Unexpected layout type {}",
tableHandle.connectorTableLayout->_type);Also applies to: 45-47, 59-65
🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 32-32: The function 'tableName' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/ClpPrestoToVeloxConnector.cpp` around lines 31 - 33, The
cast and subsequent error message access connectorSplit->_type without first
ensuring connectorSplit (and other handles like column and
tableHandle.connectorTableLayout) are non-null; change the checks around the
dynamic_cast of connectorSplit (the clpSplit variable) so you validate
connectorSplit is not null before referencing connectorSplit->_type in the
VELOX_CHECK_NOT_NULL call, and similarly guard any uses of column and
tableHandle.connectorTableLayout in the other occurrences (the checks around
lines 45-47 and 59-65) so the error path does not dereference ->_type on a null
pointer; update the VELOX_CHECK_NOT_NULL calls or add explicit null checks to
include safe, non-dereferencing messages when the incoming handle is null.
Comment on lines
+47
to
+52
| auto estimatePrecision(T timestamp) -> InputTimestampPrecision { | ||
| constexpr int64_t kEpochMilliseconds1971{31536000000}; | ||
| constexpr int64_t kEpochMicroseconds1971{31536000000000}; | ||
| constexpr int64_t kEpochNanoseconds1971{31536000000000000}; | ||
| auto absTimestamp = timestamp >= 0 ? timestamp : -timestamp; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'template <typename T>|timestamp >= 0 \? timestamp : -timestamp|convertToVeloxTimestamp\(int64_t timestamp\)|estimatePrecision\(timestamp\)' velox-connector/src/connector/search_lib/ClpTimestampsUtils.hRepository: y-scope/clp-plugin-presto-connector
Length of output: 1176
🏁 Script executed:
cat -n velox-connector/src/connector/search_lib/ClpTimestampsUtils.hRepository: y-scope/clp-plugin-presto-connector
Length of output: 6179
🏁 Script executed:
rg -n 'estimatePrecision' velox-connector/src/connector/search_lib/Repository: y-scope/clp-plugin-presto-connector
Length of output: 423
🏁 Script executed:
rg -n 'convertToVeloxTimestamp|estimatePrecision' velox-connector/src/connector/search_lib/ -A 2Repository: y-scope/clp-plugin-presto-connector
Length of output: 4114
Guard INT64_MIN in precision estimation to prevent undefined behaviour.
Line 51 can overflow for signed minimum values (-INT64_MIN), which is undefined behaviour in C++. Since convertToVeloxTimestamp(int64_t) at line 96 calls this function with untrusted integer data, an extremal timestamp can cause unpredictable behaviour.
🔧 Suggested fix
+#include <limits>
+#include <type_traits>
+
template <typename T>
auto estimatePrecision(T timestamp) -> InputTimestampPrecision {
constexpr int64_t kEpochMilliseconds1971{31536000000};
constexpr int64_t kEpochMicroseconds1971{31536000000000};
constexpr int64_t kEpochNanoseconds1971{31536000000000000};
- auto absTimestamp = timestamp >= 0 ? timestamp : -timestamp;
+ auto absTimestamp = [&]() {
+ if constexpr (std::is_floating_point_v<T>) {
+ return std::fabs(timestamp);
+ } else if constexpr (std::is_signed_v<T>) {
+ using U = std::make_unsigned_t<T>;
+ return timestamp < 0
+ ? (timestamp == std::numeric_limits<T>::min()
+ ? static_cast<U>(std::numeric_limits<T>::max()) + 1
+ : static_cast<U>(-timestamp))
+ : static_cast<U>(timestamp);
+ } else {
+ return timestamp;
+ }
+ }();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/connector/search_lib/ClpTimestampsUtils.h` around lines
47 - 52, The estimatePrecision(T timestamp) function can UB by negating
INT64_MIN; update estimatePrecision (and callers like convertToVeloxTimestamp)
to handle the extremal case explicitly instead of doing timestamp >= 0 ?
timestamp : -timestamp. Detect timestamp == std::numeric_limits<int64_t>::min()
(or use an unsigned absolute conversion) and compute absTimestamp via a safe
uint64_t path (or return the correct precision for that value) before comparing
against kEpochMilliseconds1971/kEpochMicroseconds1971/kEpochNanoseconds1971 so
no signed overflow occurs.
Comment on lines
+10
to
+12
| target_compile_features(clp_search | ||
| PRIVATE cxx_std_20 | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Consider making C++20 a PUBLIC requirement.
Since clp_search is a STATIC library with PUBLIC include directories (lines 14-19), any C++20 language features used in its public headers would require consuming targets to also compile with C++20. Using PRIVATE here means only the library's .cpp files get C++20, potentially causing compilation failures for consumers.
🔧 Suggested fix
target_compile_features(clp_search
- PRIVATE cxx_std_20
+ PUBLIC cxx_std_20
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| target_compile_features(clp_search | |
| PRIVATE cxx_std_20 | |
| ) | |
| target_compile_features(clp_search | |
| PUBLIC cxx_std_20 | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/connector/search_lib/CMakeLists.txt` around lines 10 -
12, The target_compile_features call for target clp_search currently uses
PRIVATE cxx_std_20 which only applies the C++20 requirement to the library's
implementation; change it to PUBLIC so consuming targets that include
clp_search's public headers also compile with C++20. Update the invocation of
target_compile_features(clp_search ...) to use PUBLIC cxx_std_20 to propagate
the language requirement to dependents.
Comment on lines
+6
to
+8
| target_compile_features(presto_clp_protocol | ||
| PRIVATE cxx_std_20 | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Consider making C++20 a PUBLIC requirement.
Since presto_clp_protocol is a STATIC library with PUBLIC include directories (lines 10-15), any C++20 language features used in its public headers would require consuming targets to also compile with C++20. Using PRIVATE here means only the library's .cpp files get C++20, potentially causing compilation failures for consumers.
🔧 Suggested fix
target_compile_features(presto_clp_protocol
- PRIVATE cxx_std_20
+ PUBLIC cxx_std_20
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| target_compile_features(presto_clp_protocol | |
| PRIVATE cxx_std_20 | |
| ) | |
| target_compile_features(presto_clp_protocol | |
| PUBLIC cxx_std_20 | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/protocol/CMakeLists.txt` around lines 6 - 8, The
target_compile_features call currently marks C++20 as PRIVATE for the target
named presto_clp_protocol; change it so C++20 is a PUBLIC requirement (i.e.,
make the feature requirement public) so consumers compiling against
presto_clp_protocol also get cxx_std_20; update the
target_compile_features(presto_clp_protocol ...) invocation accordingly to use
the PUBLIC specifier so public headers that use C++20 compile correctly for
dependent targets.
Comment on lines
+16
to
+20
| #include <cstdint> | ||
| #include <string> | ||
|
|
||
| #include "presto_cpp/external/json/nlohmann/json.hpp" | ||
| #include "presto_cpp/presto_protocol/core/presto_protocol_core.h" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "PrestoProtocolClp.h" -o -name "PrestoProtocolClp*"Repository: y-scope/clp-plugin-presto-connector
Length of output: 130
🏁 Script executed:
cat -n ./velox-connector/src/protocol/PrestoProtocolClp.hRepository: y-scope/clp-plugin-presto-connector
Length of output: 2312
🏁 Script executed:
cat -n ./velox-connector/src/protocol/PrestoProtocolClp.h | head -25Repository: y-scope/clp-plugin-presto-connector
Length of output: 1124
🏁 Script executed:
# Check if presto_protocol_core.h includes <memory>
find . -name "presto_protocol_core.h" 2>/dev/null | head -1Repository: y-scope/clp-plugin-presto-connector
Length of output: 61
🏁 Script executed:
find . -path "*presto_protocol/core/presto_protocol_core.h" 2>/dev/nullRepository: y-scope/clp-plugin-presto-connector
Length of output: 61
Add <memory> include to this header.
This header uses std::shared_ptr in three struct members (lines 43, 57, 58) but does not directly include <memory>, violating the "Include What You Use" principle. Relying on transitive includes is fragile and error-prone.
Proposed fix
`#include` <cstdint>
+#include <memory>
`#include` <string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #include <cstdint> | |
| #include <string> | |
| #include "presto_cpp/external/json/nlohmann/json.hpp" | |
| #include "presto_cpp/presto_protocol/core/presto_protocol_core.h" | |
| `#include` <cstdint> | |
| `#include` <memory> | |
| `#include` <string> | |
| `#include` "presto_cpp/external/json/nlohmann/json.hpp" | |
| `#include` "presto_cpp/presto_protocol/core/presto_protocol_core.h" |
🧰 Tools
🪛 Clang (14.0.6)
[error] 16-16: 'cstdint' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@velox-connector/src/protocol/PrestoProtocolClp.h` around lines 16 - 20, The
header PrestoProtocolClp.h uses std::shared_ptr in several struct members (the
shared_ptr occurrences around the members reported at lines 43, 57, 58) but
doesn't include <memory>; add `#include` <memory> to the top of
PrestoProtocolClp.h alongside the other includes so the declarations that
reference std::shared_ptr compile without relying on transitive includes.
20001020ycx
force-pushed
the
feat/2026-05-11-velox-connector-initialization
branch
from
May 12, 2026 14:53
cf2fedf to
e04e3a2
Compare
20001020ycx
force-pushed
the
feat/2026-05-11-velox-connector-initialization
branch
from
May 13, 2026 18:09
e04e3a2 to
a91b4d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@velox-connector/src/connector/CMakeLists.txt`:
- Around line 3-8: The CMake target presto_clp_connector doesn't explicitly
declare its C++ standard requirement; add a target-level requirement by
inserting a call to target_compile_features for the presto_clp_connector target
(e.g., target_compile_features(presto_clp_connector PUBLIC cxx_std_20))
immediately after the add_library(presto_clp_connector ...) block so the target
explicitly advertises C++20 to consumers and tools.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a12ad257-6343-4687-8413-7c32f30ae286
⛔ Files ignored due to path filters (1)
velox-connector/plugin.mapis excluded by!**/*.map
📒 Files selected for processing (9)
velox-connector/.gitignorevelox-connector/CMakeLists.txtvelox-connector/src/CMakeLists.txtvelox-connector/src/ClpPluginEntry.cppvelox-connector/src/connector/CMakeLists.txtvelox-connector/src/connector/search_lib/CMakeLists.txtvelox-connector/src/connector/search_lib/archive/CMakeLists.txtvelox-connector/src/connector/search_lib/ir/CMakeLists.txtvelox-connector/src/protocol/CMakeLists.txt
gibber9809
requested changes
May 13, 2026
gibber9809
left a comment
gibber9809
left a comment
Contributor
There was a problem hiding this comment.
Just reviewing the second commit with the changes to protocol/. Two minor nits, but otherwise looks good.
- Missing newline at the end of
ClpConnectorProtocol.cppandPrestoProtocolClp.hhandle. - For the various
shared_ptr<XHandle>inClpConnectorProtocol.{cpp,h}we're currently inconsistently usingporprotofor the argument name depending on whether we're actually implementing the method. For consistency and readability it might be nice to switch toprotofor all of them, or maybe a more descriptive name likehandleor something?
gibber9809
self-requested a review
May 20, 2026 17:31
gibber9809
requested changes
May 20, 2026
gibber9809
left a comment
gibber9809
left a comment
Contributor
There was a problem hiding this comment.
Just reviewing commit 3. Mostly looks good! Besides the other comments I left, I'm wondering if it might make sense to change the library names to match up a bit more with the directory structure here/change the library names to differentiate a bit better between the libraries we're pulling in from clp-s vs making in this repo.
For example, maybe clp_connector_search instead of clp_search, etc.
Co-authored-by: Devin Gibson <gibber9809@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
velox-connector/src/protocol/ClpConnectorProtocol.cpp (4)
60-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject trailing bytes after decoding each payload.
These deserializers catch under-read, but they currently allow extra trailing bytes. That can mask protocol drift or malformed payloads.
Proposed fix
+static void checkFullyConsumed(std::istringstream& in, const char* typeName) { + VELOX_CHECK(EOF == in.peek(), "Unexpected trailing bytes after {}", typeName); +} + void ClpConnectorProtocol::deserialize( const std::string& binaryData, std::shared_ptr<ColumnHandle>& proto) const { @@ handle->columnType = readUtf8String(in); + checkFullyConsumed(in, "ClpColumnHandle"); proto = handle; }Apply the same
checkFullyConsumed(...)call in the other non-empty payload deserializers.Also applies to: 80-98, 112-121, 131-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/protocol/ClpConnectorProtocol.cpp` around lines 60 - 71, The deserialize implementation in ClpConnectorProtocol (method ClpConnectorProtocol::deserialize) reads fields into a ClpColumnHandle but doesn’t reject trailing bytes; after reading columnName, originalColumnName, and columnType (using readUtf8String), call checkFullyConsumed(in) to validate there are no extra bytes and throw/handle an error if not fully consumed; apply the same checkFullyConsumed(in) pattern to the other non-empty payload deserializers mentioned (the blocks covering the other ranges) to ensure protocol drift or malformed payloads are rejected.
45-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate boolean bytes are canonical (0 or 1).
readBooleanaccepts any non-zero byte astrue. For strict wire compatibility and corruption detection, reject values outside{0,1}.Proposed fix
bool readBoolean(std::istringstream& in) { char byte{}; in.read(&byte, sizeof(byte)); VELOX_CHECK(false == in.fail(), "Failed to read boolean: insufficient bytes"); + VELOX_CHECK(byte == 0 || byte == 1, "Invalid boolean value: {}", static_cast<int>(byte)); return byte != 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/protocol/ClpConnectorProtocol.cpp` around lines 45 - 49, readBoolean currently treats any non-zero byte as true; change it to validate canonical boolean bytes by first checking the stream read succeeded (in.fail()) and then explicitly accepting only 0 or 1: return false for 0, true for 1, and otherwise fail with VELOX_CHECK (or equivalent) reporting the invalid byte value to detect wire corruption; locate the function readBoolean in ClpConnectorProtocol.cpp to implement this strict check.
156-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the zero-byte contract for transaction handle payloads.
The wire-format comment says this payload is empty, but Line 157 input is accepted without validation. Reject non-empty payloads.
Proposed fix
void ClpConnectorProtocol::deserialize( const std::string& binaryData, std::shared_ptr<ConnectorTransactionHandle>& proto) const { + VELOX_CHECK( + binaryData.empty(), + "ClpTransactionHandle expects empty payload, got {} bytes", + binaryData.size()); auto handle = std::make_shared<ClpTransactionHandle>(); handle->instance = {};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/protocol/ClpConnectorProtocol.cpp` around lines 156 - 163, The deserialize implementation for ClpConnectorProtocol currently accepts any binaryData but the wire-format requires an empty payload; update ClpConnectorProtocol::deserialize to validate that binaryData.empty() and reject non-empty payloads by throwing an appropriate exception (e.g., std::invalid_argument or runtime_error) with a clear message; keep creating the ClpTransactionHandle (handle->instance = {}) and assign proto = handle only after the empty-check; reference ClpConnectorProtocol::deserialize, ClpTransactionHandle, and proto in your change.
31-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle empty UTF-8 strings safely before buffer access.
Line 32 uses
&result[0]even whenlength == 0, which is undefined behaviour for an emptystd::string. Guard the read before taking a writable buffer pointer.Proposed fix
std::string result(length, '\0'); - in.read(&result[0], length); + if (0 < length) { + in.read(result.data(), length); + } VELOX_CHECK(false == in.fail(), "Failed to read UTF body: insufficient bytes");#!/bin/bash # Verify potential empty-string writable-buffer access in this file. rg -n --type=cpp -C2 '&result\[0\]' velox-connector/src/protocol/ClpConnectorProtocol.cpp🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/src/protocol/ClpConnectorProtocol.cpp` around lines 31 - 33, The code reads into result via in.read(&result[0], length) without guarding length==0, which can invoke undefined behaviour; update the logic around result, length and in.read so you only call in.read with a writable buffer when length > 0 (e.g., skip the in.read when length==0 and leave result as empty), and keep or move the VELOX_CHECK("Failed to read UTF body: insufficient bytes") to validate the stream read outcome after attempting the read; refer to the variables result, length and the in.read call to locate and fix the issue.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@velox-connector/CMakeLists.txt`:
- Around line 11-20: Replace the global modification of CMAKE_CXX_FLAGS that
adds -mavx2 with a target-scoped compile option for the plugin target
(presto_clp_plugin) so only that target gets AVX2 enabled; remove or revert the
set(CMAKE_CXX_FLAGS ...) change and add a
target_compile_options(presto_clp_plugin PRIVATE -mavx2) for the plugin, and add
a short comment documenting the x86_64/AVX2 platform requirement near the
presto_clp_plugin configuration.
---
Outside diff comments:
In `@velox-connector/src/protocol/ClpConnectorProtocol.cpp`:
- Around line 60-71: The deserialize implementation in ClpConnectorProtocol
(method ClpConnectorProtocol::deserialize) reads fields into a ClpColumnHandle
but doesn’t reject trailing bytes; after reading columnName, originalColumnName,
and columnType (using readUtf8String), call checkFullyConsumed(in) to validate
there are no extra bytes and throw/handle an error if not fully consumed; apply
the same checkFullyConsumed(in) pattern to the other non-empty payload
deserializers mentioned (the blocks covering the other ranges) to ensure
protocol drift or malformed payloads are rejected.
- Around line 45-49: readBoolean currently treats any non-zero byte as true;
change it to validate canonical boolean bytes by first checking the stream read
succeeded (in.fail()) and then explicitly accepting only 0 or 1: return false
for 0, true for 1, and otherwise fail with VELOX_CHECK (or equivalent) reporting
the invalid byte value to detect wire corruption; locate the function
readBoolean in ClpConnectorProtocol.cpp to implement this strict check.
- Around line 156-163: The deserialize implementation for ClpConnectorProtocol
currently accepts any binaryData but the wire-format requires an empty payload;
update ClpConnectorProtocol::deserialize to validate that binaryData.empty() and
reject non-empty payloads by throwing an appropriate exception (e.g.,
std::invalid_argument or runtime_error) with a clear message; keep creating the
ClpTransactionHandle (handle->instance = {}) and assign proto = handle only
after the empty-check; reference ClpConnectorProtocol::deserialize,
ClpTransactionHandle, and proto in your change.
- Around line 31-33: The code reads into result via in.read(&result[0], length)
without guarding length==0, which can invoke undefined behaviour; update the
logic around result, length and in.read so you only call in.read with a writable
buffer when length > 0 (e.g., skip the in.read when length==0 and leave result
as empty), and keep or move the VELOX_CHECK("Failed to read UTF body:
insufficient bytes") to validate the stream read outcome after attempting the
read; refer to the variables result, length and the in.read call to locate and
fix the issue.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 792f05d3-739f-4fa9-96e8-5ebc553feb32
📒 Files selected for processing (9)
velox-connector/.gitignorevelox-connector/CMakeLists.txtvelox-connector/src/connector/CMakeLists.txtvelox-connector/src/connector/search_lib/CMakeLists.txtvelox-connector/src/connector/search_lib/archive/CMakeLists.txtvelox-connector/src/connector/search_lib/ir/CMakeLists.txtvelox-connector/src/protocol/ClpConnectorProtocol.cppvelox-connector/src/protocol/ClpConnectorProtocol.hvelox-connector/src/protocol/PrestoProtocolClp.h
Collaborator
Author
Addressed all comments, please see the changes that resolve your comment in here . Also, agree with your library name change proposal, this is a good catch. Renamed I have performed e2e testing with the proposed change (base image + our plugin, see PR description for more details), the query returns result as expected. Also noticed a regression while doing the testing, the Again, appreciate for the speedy and insightful review :) |
gibber9809
previously approved these changes
May 21, 2026
gibber9809
left a comment
gibber9809
left a comment
Contributor
There was a problem hiding this comment.
Commit 2&3+changes addressing my comments on them LGTM!
For the PR title maybe something like:
feat: Add C++ Presto Worker plugin.
| @@ -0,0 +1,110 @@ | |||
| cmake_minimum_required(VERSION 3.28) | |||
Member
There was a problem hiding this comment.
Any reasoning for the choice of 3.28?
Collaborator
Author
There was a problem hiding this comment.
Good question. We've always been using this version, and I think it comes from the requirement when building velox: https://github.com/facebookincubator/velox/blob/main/CMakeLists.txt
The Presto worker uses 3.10 and CLP uses anything later than 3.23. So i guess the best way to mitigate is to use what velox has required - 3.28. At lease emprically this version didn't lead to any problem.
9 tasks done
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Collaborator
Author
|
Addressed all comments from @kirkrodrigues , also updated the shared library name to |
…ub.com:20001020ycx/clp-plugin-presto-connector into feat/2026-05-11-velox-connector-initialization
9 tasks done
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
kirkrodrigues
approved these changes
May 22, 2026
kirkrodrigues
left a comment
kirkrodrigues
left a comment
Member
There was a problem hiding this comment.
Deferring to @gibber9809's review.
9 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is the second PR in a series migrating our OSS Presto CLP connector from the forked repository to this standalone plugin repository. This PR ports C++ plugin for the Presto worker from our forked repository (branch
presto-0.297-edge-10-clp-connector)Reviewer's note
Since this migration involves substantial changes, I have split them across three commits for easier review, below listed the brief summary of each commit along with some design note to help review:
Commit 1 ( no content review needed): This is an exact source code of the OSS C++ plugin for the Presto worker from the branch
presto-0.297-edge-10-clp-connectorin our forked repository.Commit 2: Migrate Presto worker deserialization protocol to the plugin.
All source code under
protocol/is copied from this PR and modified to address concerns discussed there:ClpConnectorProtocol.cpp: exactly copy from this PR.ClpConnectorProtocol.h: marks allto_json/from_jsonmethods as NYI as motivated in this PR; as plugin does not support JSON serialization/deserialization.PrestoProtocolClp.h: replaces the old auto-generatedpresto_protocol_clp.h(previously generated by chevron). Now, this is a hand-written file that only defines the data structures for binary serialization, and is free of all generated code bypresto_protocol_clp.hbecause we have decoupled the Presto Native Worker Protocol Code Generation with binary serialization, please see this PR for more details. Also applied the change from this discussion.Commit 3: Add all CMake files, and the plugin entry point:
dlopenon this plugin; this is possible because we package the plugin as an ELF share library.taskcommand.ClpPluginEntry.cpp: plugin entry point, following this RFC.plugin.mapexplicitly exports this function so the Presto worker can registers theclpconnector factory throughdlopen(Note, this is required because plugin uses hidden visibility to avoid dependency version conflicts).CMakeLists.txtundertestsdirectory is intentionally omitted. This is because linking against Velox's public testing libraries (velox_exec_test_libandvelox_vector_test_lib) pulls way many transitive dependencies from Velox that our plugin does not manage. This leaves us no choice but reusing our forked repository as the infra to run the unit test, which will be implemented in the later PR about Taskfile.Breaking changes
None
Validation performed
make -j) succeed, provided all dependency are properly installed.SELECT timestamp, CLP_GET_JSON_STRING() from clp.default.default limit 100Checklist
(fixes #N)or(resolves #N)syntax in the title.Summary by CodeRabbit
New Features
Tests
Chores