Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
5af7ec3
Extract ClpCursor abstract class and move the original ClpCursor impl…
anlowee Aug 19, 2025
fe8a130
Merge the code
anlowee Aug 19, 2025
cc82dd7
Rename the extracrted ClpCursor to BaseClpCursor, and add the skeleto…
anlowee Aug 19, 2025
65d3bd1
WIP: implement the ClpIrCursor, one todo is to marshal the row in Clp…
anlowee Aug 19, 2025
d45baf0
Remove the IR related code and add a subpackage to store archive sear…
anlowee Aug 20, 2025
7f40b7e
Address coderabbitai comments
anlowee Aug 20, 2025
c0cac3d
Address comments
anlowee Aug 20, 2025
c7bd83b
Update from part1 branch
anlowee Aug 27, 2025
27ba897
WIP
anlowee Sep 2, 2025
19bad61
Move the current VectorLoader to Archive-specific
anlowee Sep 2, 2025
d3fafde
Merge branch 'xwei/ir-integration-phase2-part1' into xwei/ir-integrat…
anlowee Sep 2, 2025
f53c6c7
Rename and WIP
anlowee Sep 3, 2025
e466f68
Put more archive specific thing into Archive code
anlowee Sep 3, 2025
8be8eb7
Merge branch 'xwei/ir-integration-phase2-part1' into xwei/ir-integrat…
anlowee Sep 3, 2025
d6ba0da
WIP
anlowee Sep 3, 2025
221bec6
WIP
anlowee Sep 3, 2025
679d338
Merge branch 'presto-0.293-clp-connector' into xwei/ir-integration-ph…
anlowee Sep 3, 2025
6e1462d
Merge branch 'xwei/ir-integration-phase2-part1' into xwei/ir-integrat…
anlowee Sep 3, 2025
f8f75c3
Get it work
anlowee Sep 4, 2025
1c091c7
Code clean up
anlowee Sep 4, 2025
8ad9b9c
Merge branch 'xwei/ir-integration-phase2-part1' into xwei/ir-integrat…
anlowee Sep 4, 2025
7f25210
Address coderabbitai comments
anlowee Sep 4, 2025
28e9a6f
Merge branch 'xwei/ir-integration-phase2-part1' into xwei/ir-integrat…
anlowee Sep 4, 2025
595e74b
Minor fix
anlowee Sep 4, 2025
1b12402
Fix unitest
anlowee Sep 4, 2025
5db1cf9
Fix building issues
anlowee Sep 5, 2025
e71a957
Address comments
anlowee Sep 5, 2025
cc82c62
Merge branch 'xwei/ir-integration-phase2-part1' into xwei/ir-integrat…
anlowee Sep 5, 2025
3ca7361
Try out zhihao fix
anlowee Sep 5, 2025
184f9de
Apply Zhihao fix
anlowee Sep 5, 2025
2102275
Merge branch 'presto-0.293-clp-connector' into xwei/ir-integration-ph…
anlowee Sep 5, 2025
f9e1409
Address some coderabbitai comments after merging the first PR
anlowee Sep 5, 2025
36c02c7
Address comments
anlowee Sep 9, 2025
ee14262
Address a comment
anlowee Sep 9, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMake/resolve_dependency_modules/clp.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ include_guard(GLOBAL)
FetchContent_Declare(
clp
GIT_REPOSITORY https://github.com/y-scope/clp.git
GIT_TAG 19cd534e629d746395efc64343a60f768b0c9a2d)
GIT_TAG bfd4f60ffe9c5d69618cc8416ec6729c76ee9862)

Comment thread
anlowee marked this conversation as resolved.
set(CLP_BUILD_CLP_REGEX_UTILS
OFF
Expand Down
1 change: 1 addition & 0 deletions CMake/resolve_dependency_modules/ystdlib_cpp.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,4 @@ FetchContent_Declare(
FetchContent_Populate(ystdlib_cpp)

set(CLP_YSTDLIB_SOURCE_DIRECTORY "${ystdlib_cpp_SOURCE_DIR}")
include_directories(${ystdlib_cpp_SOURCE_DIR}/src)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Avoid global include_directories; mark third-party headers as SYSTEM.

Use SYSTEM (and optionally BEFORE) to reduce warning noise and unintended global leakage.

-include_directories(${ystdlib_cpp_SOURCE_DIR}/src)
+include_directories(SYSTEM BEFORE ${ystdlib_cpp_SOURCE_DIR}/src)
📝 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_directories(${ystdlib_cpp_SOURCE_DIR}/src)
include_directories(SYSTEM BEFORE ${ystdlib_cpp_SOURCE_DIR}/src)
🤖 Prompt for AI Agents
In CMake/resolve_dependency_modules/ystdlib_cpp.cmake around line 24, avoid the
global include_directories call; instead mark the third-party headers as SYSTEM
and attach them to the appropriate target. Replace the global
include_directories(${ystdlib_cpp_SOURCE_DIR}/src) with either use
target_include_directories(<your_target> SYSTEM BEFORE PUBLIC
${ystdlib_cpp_SOURCE_DIR}/src) (preferred) or, if a target is not available, at
minimum use include_directories(SYSTEM ${ystdlib_cpp_SOURCE_DIR}/src) so the
headers are treated as system headers and warnings are suppressed and global
leakage is reduced.

4 changes: 4 additions & 0 deletions velox/connectors/clp/ClpDataSource.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "velox/connectors/clp/ClpTableHandle.h"
#include "velox/connectors/clp/search_lib/ClpS3AuthProviderBase.h"
#include "velox/connectors/clp/search_lib/archive/ClpArchiveCursor.h"
#include "velox/connectors/clp/search_lib/ir/ClpIrCursor.h"
#include "velox/vector/FlatVector.h"

namespace facebook::velox::connector::clp {
Expand Down Expand Up @@ -116,6 +117,9 @@ void ClpDataSource::addSplit(std::shared_ptr<ConnectorSplit> split) {
if (ClpConnectorSplit::SplitType::kArchive == clpSplit->type_) {
cursor_ =
std::make_unique<search_lib::ClpArchiveCursor>(inputSource, splitPath);
} else if (ClpConnectorSplit::SplitType::kIr == clpSplit->type_) {
cursor_ =
std::make_unique<search_lib::ClpIrCursor>(inputSource, splitPath, true);
} else {
VELOX_UNSUPPORTED(
"Unsupported split type: {}", static_cast<int>(clpSplit->type_));
Expand Down
6 changes: 5 additions & 1 deletion velox/connectors/clp/search_lib/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ velox_add_library(
ClpS3AuthProviderBase.h)

add_subdirectory(archive)
add_subdirectory(ir)

velox_link_libraries(clp-s-search PUBLIC clp-s-archive-search)
velox_link_libraries(
clp-s-search
PUBLIC clp-s-archive-search
PUBLIC clp-s-ir-search)

target_compile_features(clp-s-search PRIVATE cxx_std_20)
33 changes: 33 additions & 0 deletions velox/connectors/clp/search_lib/ir/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Copyright (c) Facebook, Inc. and its affiliates.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
velox_add_library(
clp-s-ir-search
STATIC
ClpIrCursor.cpp
ClpIrCursor.h
ClpIrUnitHandler.h
ClpIrVectorLoader.cpp
ClpIrVectorLoader.h)

velox_link_libraries(
clp-s-ir-search
PUBLIC clp_s::archive_reader
PRIVATE
# Once the IR-stream-related targets are exported, this should be updated to
# use more fine-grained dependencies.
clp_s::clp_dependencies

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make a note to export library targets for the IR stream code inside of the CLP repo -- relying on clp_s::clp_dependencies will work for now, but isn't ideal long term.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about I adding a comment "# After IR-stream-related targets are exported, we should replace this with finer granularity dependencies" above clp_s::clp_dependencies

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# Once the IR-stream-related targets are exported, this should be updated to use more fine-grained dependencies.

clp_s::io
clp_s::search
clp_s::search::kql
velox_vector)
211 changes: 211 additions & 0 deletions velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "clp_s/ColumnReader.hpp"
#include "clp_s/InputConfig.hpp"

#include "ffi/ir_stream/search/QueryHandler.hpp"
#include "velox/connectors/clp/search_lib/ir/ClpIrCursor.h"
#include "velox/connectors/clp/search_lib/ir/ClpIrVectorLoader.h"

using namespace clp_s;

namespace facebook::velox::connector::clp::search_lib {
Comment thread
coderabbitai[bot] marked this conversation as resolved.

uint64_t ClpIrCursor::fetchNext(uint64_t numRows) {
readerIndex_ = 0;
if (ErrorCode::Success != errorCode_) {
return 0;
}

if (false == currentSplitLoaded_) {
errorCode_ = loadSplit();
if (ErrorCode::Success != errorCode_) {
return 0;
}
}

auto deserializeResult = deserialize(numRows);
if (deserializeResult.has_error()) {
auto error = deserializeResult.error();
VELOX_FAIL(
"IR file {} might be broken, failed to deserialize. {}: {}",
this->splitPath_,
error.category().name(),
error.message());
}
return irDeserializer_->get_num_log_events_deserialized();
}
Comment thread
anlowee marked this conversation as resolved.
Comment thread
anlowee marked this conversation as resolved.

size_t ClpIrCursor::getNumFilteredRows() const {
return irDeserializer_->get_ir_unit_handler().getFilteredLogEvents()->size();
}

VectorPtr ClpIrCursor::createVector(
memory::MemoryPool* pool,
const TypePtr& vectorType,
size_t vectorSize) {
VELOX_CHECK_EQ(
projectedColumnIdxNodeIdMap_.size(),
outputColumns_.size(),
"Projected columns size {} does not match fields size {}",
projectedColumnIdxNodeIdMap_.size(),
outputColumns_.size());
return createVectorHelper(pool, vectorType, vectorSize);
}

ErrorCode ClpIrCursor::loadSplit() {
auto networkAuthOption = inputSource_ == InputSource::Filesystem
? NetworkAuthOption{.method = AuthMethod::None}
: NetworkAuthOption{.method = AuthMethod::S3PresignedUrlV4};

auto irHandler = ClpIrUnitHandler{};

auto projections = splitFieldsToNamesAndTypes();
auto queryHandlerResult{QueryHandlerType::create(
projectionResolutionCallback_,
std::move(expr_),
projections,
ignoreCase_)};
if (!queryHandlerResult) {
VLOG(2) << "Failed to create query handler for deserialization.";
return ErrorCode::InternalError;
}
auto queryHandler = std::move(queryHandlerResult).value();

auto irPath = Path{.source = inputSource_, .path = splitPath_};
irReader_ = try_create_reader(irPath, networkAuthOption);
if (nullptr == irReader_) {
VLOG(2) << "Failed to open kv-ir stream \"" << splitPath_
<< "\" for reading.";
return ErrorCode::InternalError;
}

auto deserializerResult = ::clp::ffi::ir_stream::make_deserializer(
*irReader_, std::move(irHandler), std::move(queryHandler));
if (!deserializerResult) {
VLOG(2) << "Failed to create deserializer for deserialization.";
return ErrorCode::InternalError;
}
irDeserializer_ = std::make_shared<
::clp::ffi::ir_stream::Deserializer<ClpIrUnitHandler, QueryHandlerType>>(
std::move(deserializerResult).value());

currentSplitLoaded_ = true;
return ErrorCode::Success;
}

std::vector<std::pair<std::string, search::ast::literal_type_bitmask_t>>
ClpIrCursor::splitFieldsToNamesAndTypes() const {
auto result = std::vector<
std::pair<std::string, search::ast::literal_type_bitmask_t>>{};
for (size_t i{0}; i < outputColumns_.size(); ++i) {
auto column = outputColumns_[i];
search::ast::literal_type_bitmask_t literalType;
switch (column.type) {
case ColumnType::Array:
literalType = search::ast::LiteralType::ArrayT;
break;
case ColumnType::Boolean:
literalType = search::ast::LiteralType::BooleanT;
break;
case ColumnType::Float:
literalType = search::ast::LiteralType::FloatT;
break;
case ColumnType::Integer:
literalType = search::ast::LiteralType::IntegerT;
break;
case ColumnType::String:
literalType = search::ast::LiteralType::VarStringT |
search::ast::LiteralType::ClpStringT;
break;
case ColumnType::Timestamp:
// TODO: IR timestamp support pending; constrain to Unknown to avoid
// mismatched projections.
literalType = search::ast::LiteralType::EpochDateT;
break;
default:
literalType = search::ast::LiteralType::UnknownT;
break;
}
result.emplace_back(column.name, literalType);
}
return result;
}

ystdlib::error_handling::Result<void> ClpIrCursor::deserialize(
uint64_t numRows) {
irDeserializer_->get_ir_unit_handler().clearFilteredLogEvents();
uint64_t cnt{0};
while (cnt < numRows) {
auto deserializeResult =
irDeserializer_->deserialize_next_ir_unit(*irReader_);
if (deserializeResult.has_error()) {
auto error = deserializeResult.error();
if (std::errc::result_out_of_range == error ||
irDeserializer_->is_stream_completed()) {
break;
}
return error;
Comment on lines +156 to +162

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Judging by some comments in the clp code it looks like we might switch from result_out_of_range to another error code to indicate truncated IR streams in the future -- we should probably put up an issue or something to track that so that we remember to change this code at the same time.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LinZhihao-723 Do we have such an issue?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We were planning to add our own error code for IR serde. I think we will have it in at some point.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have an issue link so that I can put it in the code comment?

}
if (::clp::ffi::ir_stream::IrUnitType::LogEvent ==
deserializeResult.value()) {
++cnt;
}
}
return ystdlib::error_handling::success();
}

VectorPtr ClpIrCursor::createVectorHelper(
memory::MemoryPool* pool,
const TypePtr& vectorType,
size_t vectorSize) {
if (vectorType->kind() == TypeKind::ROW) {
std::vector<VectorPtr> children;
auto& rowType = vectorType->as<TypeKind::ROW>();
for (uint32_t i = 0; i < rowType.size(); ++i) {
children.push_back(
createVectorHelper(pool, rowType.childAt(i), vectorSize));
}
Comment on lines +177 to +182

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Pre-reserve children vector capacity.

Minor allocation win for ROW vectors.

-    std::vector<VectorPtr> children;
+    std::vector<VectorPtr> children;
+    children.reserve(rowType.size());

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp around lines 173 to 178,
the children vector is built by repeatedly pushing back without reserving
capacity which causes extra allocations; call children.reserve(rowType.size())
before the loop (use appropriate integer type for reserve if needed) so the
vector pre-allocates space for all ROW child vectors and avoids repeated
reallocations.

return std::make_shared<RowVector>(
pool, vectorType, nullptr, vectorSize, std::move(children));
}
auto vector = BaseVector::create(vectorType, vectorSize, pool);
vector->setNulls(allocateNulls(vectorSize, pool, bits::kNull));
VELOX_CHECK_LT(
readerIndex_, outputColumns_.size(), "Reader index out of bounds");
auto projectedColumn = outputColumns_[readerIndex_];
auto projectedColumnType = projectedColumn.type;
auto it = projectedColumnIdxNodeIdMap_.find(readerIndex_);
bool isResolved = it != projectedColumnIdxNodeIdMap_.end();
::clp::ffi::SchemaTree::Node::id_t projectedColumnNodeId;
if (isResolved) {
projectedColumnNodeId = it->second;
}
readerIndex_++;
return std::make_shared<LazyVector>(
pool,
vectorType,
vectorSize,
std::make_unique<ClpIrVectorLoader>(
isResolved,
projectedColumnType,
projectedColumnNodeId,
irDeserializer_->get_ir_unit_handler().getFilteredLogEvents()),
std::move(vector));
Comment on lines 194 to 208

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use of uninitialised projectedColumnNodeId when unresolved.

projectedColumnNodeId is only set when isResolved is true, but passed unconditionally to ClpIrVectorLoader.

-  ::clp::ffi::SchemaTree::Node::id_t projectedColumnNodeId;
-  if (isResolved) {
-    projectedColumnNodeId = it->second;
-  }
+  ::clp::ffi::SchemaTree::Node::id_t projectedColumnNodeId{};
+  if (isResolved) {
+    projectedColumnNodeId = it->second;
+  }
📝 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
::clp::ffi::SchemaTree::Node::id_t projectedColumnNodeId;
if (isResolved) {
projectedColumnNodeId = it->second;
}
readerIndex_++;
return std::make_shared<LazyVector>(
pool,
vectorType,
vectorSize,
std::make_unique<ClpIrVectorLoader>(
isResolved,
projectedColumnType,
projectedColumnNodeId,
irDeserializer_->get_ir_unit_handler().getFilteredLogEvents()),
std::move(vector));
::clp::ffi::SchemaTree::Node::id_t projectedColumnNodeId{};
if (isResolved) {
projectedColumnNodeId = it->second;
}
readerIndex_++;
return std::make_shared<LazyVector>(
pool,
vectorType,
vectorSize,
std::make_unique<ClpIrVectorLoader>(
isResolved,
projectedColumnType,
projectedColumnNodeId,
irDeserializer_->get_ir_unit_handler().getFilteredLogEvents()),
std::move(vector));
🤖 Prompt for AI Agents
In velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp around lines 191 to 205,
projectedColumnNodeId is only assigned when isResolved is true but is
unconditionally passed to ClpIrVectorLoader; initialize projectedColumnNodeId at
declaration to a safe default (e.g. ::clp::ffi::SchemaTree::Node::id_t{} or an
explicit INVALID sentinel) or otherwise ensure it’s set for the unresolved path,
so the loader always receives a well-defined value (alternatively, modify
ClpIrVectorLoader to accept an optional/flag indicating unresolved state and
pass that instead).

}

} // namespace facebook::velox::connector::clp::search_lib
89 changes: 89 additions & 0 deletions velox/connectors/clp/search_lib/ir/ClpIrCursor.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#pragma once

#include "ffi/ir_stream/Deserializer.hpp"
#include "velox/connectors/clp/search_lib/BaseClpCursor.h"
#include "velox/connectors/clp/search_lib/ir/ClpIrUnitHandler.h"

Comment on lines +19 to +22

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Forward-declare clp::ReaderInterface to satisfy pointer usage.

You declare a member std::shared_ptr<::clp::ReaderInterface> without including or forward-declaring the type. Add a forward declaration to avoid hard dependency and fix compilation.

 #include "velox/connectors/clp/search_lib/ir/ClpIrUnitHandler.h"

+namespace clp {
+class ReaderInterface;
+} // namespace clp
📝 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 "ffi/ir_stream/Deserializer.hpp"
#include "velox/connectors/clp/search_lib/BaseClpCursor.h"
#include "velox/connectors/clp/search_lib/ir/ClpIrUnitHandler.h"
#include "ffi/ir_stream/Deserializer.hpp"
#include "velox/connectors/clp/search_lib/BaseClpCursor.h"
#include "velox/connectors/clp/search_lib/ir/ClpIrUnitHandler.h"
namespace clp {
class ReaderInterface;
} // namespace clp
🤖 Prompt for AI Agents
In velox/connectors/clp/search_lib/ir/ClpIrCursor.h around lines 19-22 you use
std::shared_ptr<::clp::ReaderInterface> but never forward-declare
ReaderInterface, causing a hard dependency/compilation error; add a forward
declaration "namespace clp { class ReaderInterface; }" near the top of the
header (after the includes and before the ClpIrCursor class declaration) so the
pointer type is known without including the full ReaderInterface header.

⚠️ Potential issue

Add required standard headers; avoid relying on transitive includes.

This header uses std::string, std::string_view, std::vector, std::unordered_map, std::shared_ptr, std::function, and std::pair but doesn’t include their headers. This can break downstream builds.

 #pragma once

+#include <functional>
+#include <memory>
+#include <string>
+#include <string_view>
+#include <unordered_map>
+#include <utility>
+#include <vector>
+
 #include "ffi/ir_stream/Deserializer.hpp"
 #include "velox/connectors/clp/search_lib/BaseClpCursor.h"
 #include "velox/connectors/clp/search_lib/ir/ClpIrUnitHandler.h"
🤖 Prompt for AI Agents
In velox/connectors/clp/search_lib/ir/ClpIrCursor.h around lines 19 to 22, the
header uses std::string, std::string_view, std::vector, std::unordered_map,
std::shared_ptr, std::function, and std::pair but doesn’t include their headers;
add the missing standard includes at the top of the file: #include <string>,
#include <string_view>, #include <vector>, #include <unordered_map>, #include
<memory>, #include <functional>, and #include <utility> so the file does not
rely on transitive includes and downstream builds remain stable.

namespace facebook::velox::connector::clp::search_lib {

class ClpIrCursor final : public BaseClpCursor {
public:
explicit ClpIrCursor(
clp_s::InputSource inputSource,
std::string_view splitPath,
bool ignoreCase)
: BaseClpCursor(inputSource, splitPath), ignoreCase_(ignoreCase) {}

ClpIrCursor(const ClpIrCursor&) = delete;
ClpIrCursor& operator=(const ClpIrCursor&) = delete;
ClpIrCursor(ClpIrCursor&&) = delete;
ClpIrCursor& operator=(ClpIrCursor&&) = delete;

uint64_t fetchNext(uint64_t numRows) override;

size_t getNumFilteredRows() const override;

VectorPtr createVector(
memory::MemoryPool* pool,
const TypePtr& vectorType,
size_t vectorSize) override;

protected:
ErrorCode loadSplit() override;

Comment thread
anlowee marked this conversation as resolved.
private:
std::function<ystdlib::error_handling::Result<void>(
bool,
::clp::ffi::SchemaTree::Node::id_t,
std::pair<std::string_view, size_t>)>
projectionResolutionCallback_ =
[this](
[[maybe_unused]] bool isAutoGenerated,
[[maybe_unused]] ::clp::ffi::SchemaTree::Node::id_t nodeId,
[[maybe_unused]] std::pair<std::string_view, size_t>
projected_key_and_index)
-> ystdlib::error_handling::Result<void> {
projectedColumnIdxNodeIdMap_.insert(
{projected_key_and_index.second, nodeId});
return ystdlib::error_handling::success();
};
using QueryHandlerType = ::clp::ffi::ir_stream::search::QueryHandler<
decltype(projectionResolutionCallback_)>;
bool ignoreCase_;
std::shared_ptr<
::clp::ffi::ir_stream::Deserializer<ClpIrUnitHandler, QueryHandlerType>>
irDeserializer_;
std::shared_ptr<::clp::ReaderInterface> irReader_{nullptr};
std::unordered_map<size_t, ::clp::ffi::SchemaTree::Node::id_t>
projectedColumnIdxNodeIdMap_;
size_t readerIndex_{0};

std::vector<
std::pair<std::string, clp_s::search::ast::literal_type_bitmask_t>>
splitFieldsToNamesAndTypes() const;
Comment on lines 77 to 79

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Unify type qualifier for literal_type_bitmask_t in decl/def and add the direct include.

Header uses clp_s::search::ast::literal_type_bitmask_t, but the .cpp shows search::ast::literal_type_bitmask_t. Make them consistent (prefer fully-qualified clp_s::search::ast::...).

Proposed .cpp fix:

-std::vector<std::pair<std::string, search::ast::literal_type_bitmask_t>>
+std::vector<std::pair<std::string, clp_s::search::ast::literal_type_bitmask_t>>
 ClpIrCursor::splitFieldsToNamesAndTypes() const {

Verification script:


🏁 Script executed:

#!/bin/bash
# Check for inconsistent qualifiers between header and source.
rg -n -C2 'literal_type_bitmask_t' velox/connectors/clp/search_lib/ir/ClpIrCursor.{h,cpp}
# Check if an alias like `using search = clp_s::search` exists.
rg -n -C2 -P 'using\s+search\s*=\s*clp_s::search' velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp

Length of output: 1422


Unify literal_type_bitmask_t qualifier and add its direct include

  • Header declares splitFieldsToNamesAndTypes() returning std::vector<std::pair<std::string, clp_s::search::ast::literal_type_bitmask_t>>, but the .cpp defines and uses search::ast::literal_type_bitmask_t. Update the .cpp signature and all occurrences to clp_s::search::ast::literal_type_bitmask_t.
  • Include the header that defines literal_type_bitmask_t directly in ClpIrCursor.cpp.
🤖 Prompt for AI Agents
In velox/connectors/clp/search_lib/ir/ClpIrCursor.h around lines 77 to 79, the
header declares splitFieldsToNamesAndTypes() returning
std::vector<std::pair<std::string, clp_s::search::ast::literal_type_bitmask_t>>
but the implementation and usages in ClpIrCursor.cpp still reference
search::ast::literal_type_bitmask_t; update the ClpIrCursor.cpp function
signature and all occurrences to use the fully-qualified
clp_s::search::ast::literal_type_bitmask_t to match the header, and add an
#include for the header that defines literal_type_bitmask_t at the top of
ClpIrCursor.cpp so the type is resolved directly.


ystdlib::error_handling::Result<void> deserialize(uint64_t numRows);

VectorPtr createVectorHelper(
memory::MemoryPool* pool,
const TypePtr& vectorType,
size_t vectorSize);
};

} // namespace facebook::velox::connector::clp::search_lib
Loading
Loading