Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions velox/connectors/clp/ClpDataSource.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ void ClpDataSource::addFieldsRecursively(
case TypeKind::ARRAY:
clpColumnType = search_lib::ColumnType::Array;
break;
case TypeKind::TIMESTAMP:
clpColumnType = search_lib::ColumnType::Timestamp;
break;
default:
VELOX_USER_FAIL("Type not supported: {}", columnType->name());
}
Expand Down
5 changes: 5 additions & 0 deletions velox/connectors/clp/search_lib/ClpCursor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,11 @@ ErrorCode ClpCursor::loadArchive() {
case ColumnType::Array:
columnDescriptor->set_matching_types(LiteralType::ArrayT);
break;
case ColumnType::Timestamp:
columnDescriptor->set_matching_types(
LiteralType::EpochDateT | LiteralType::IntegerT |
LiteralType::FloatT);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
break;
default:
break;
}
Expand Down
10 changes: 9 additions & 1 deletion velox/connectors/clp/search_lib/ClpCursor.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,15 @@ enum class ErrorCode {
Success
};

enum class ColumnType { String, Integer, Float, Array, Boolean, Unknown = -1 };
enum class ColumnType {
Array,
Boolean,
Float,
Integer,
String,
Timestamp,
Unknown = -1
};

struct Field {
ColumnType type;
Expand Down
172 changes: 172 additions & 0 deletions velox/connectors/clp/search_lib/ClpVectorLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,113 @@
* limitations under the License.
*/

#include <cmath>
#include <cstdint>
#include <utility>

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

@coderabbitai coderabbitai Bot Jul 10, 2025

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)

Remove unused include.

The include "clp_s/SchemaTree.hpp" appears to be unused in this file.

-#include "clp_s/SchemaTree.hpp"
📝 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 "clp_s/SchemaTree.hpp"
#include <cmath>
#include <cstdint>
#include "velox/type/Timestamp.h"
🤖 Prompt for AI Agents
In velox/connectors/clp/search_lib/ClpVectorLoader.cpp at line 22, the include
directive for "clp_s/SchemaTree.hpp" is unused. Remove this include statement to
clean up the code and avoid unnecessary 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.

It is used for clp_s::NodeType

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for the definition of clp_s::NodeType to determine which header file defines it
echo "Searching for clp_s::NodeType definition..."
rg -A 3 -B 3 "enum.*NodeType" --type cpp --type hpp
echo ""
echo "Searching for NodeType in SchemaTree.hpp specifically..."
rg -A 10 -B 5 "NodeType" --files-with-matches | grep -i schema
echo ""
echo "Searching for files containing NodeType..."
rg -l "NodeType" --type cpp --type hpp

Length of output: 601


🏁 Script executed:

#!/bin/bash
# Search more broadly for NodeType definition and usage
echo "Searching for NodeType definition..."
rg -A 5 -B 5 "enum.*NodeType" --type cpp --type c
echo ""
echo "Searching for NodeType in header files..."
rg -A 5 -B 5 "enum.*NodeType" --type h
echo ""
echo "Searching for all files containing NodeType..."
rg -l "NodeType" --type cpp --type c --type h
echo ""
echo "Searching specifically for clp_s::NodeType..."
rg -A 5 -B 5 "clp_s::NodeType" --type cpp --type c --type h
echo ""
echo "Searching for include statements with SchemaTree..."
rg "#include.*SchemaTree" --type cpp --type c --type h

Length of output: 8722


@gibber9809 You're absolutely right, and I apologize for the incorrect assessment. Looking at the code more carefully, I can see that clp_s::NodeType is used extensively throughout the file:

  • As a template parameter: template <clp_s::NodeType Type>
  • In case statements: clp_s::NodeType::Float, clp_s::NodeType::Integer, clp_s::NodeType::DateString, clp_s::NodeType::Unknown
  • In function calls: populateTimestampData<clp_s::NodeType::Integer>(rows, timestampVector)
  • In explicit template instantiations at the end of the file

The #include "clp_s/SchemaTree.hpp" is clearly needed to provide the definition of the clp_s::NodeType enum. Thank you for the correction!


🧬 Code Graph Analysis Results

Relevant snippets from velox/connectors/clp/search_lib/ClpVectorLoader.h (lines 38-56):

ClpVectorLoader(
      clp_s::BaseColumnReader* columnReader,
      ColumnType nodeType,
      std::shared_ptr<std::vector<uint64_t>> filteredRowIndices);

void loadInternal(
      RowSet rows,
      ValueHook* hook,
      vector_size_t resultSize,
      VectorPtr* result) override;

template <typename T, typename VectorPtr>
void populateData(RowSet rows, VectorPtr vector);

template <clp_s::NodeType Type>
void populateTimestampData(
      RowSet rows,
      FlatVector<facebook::velox::Timestamp>* vector);
  • The constructor takes a pointer to clp_s::BaseColumnReader, a ColumnType (which corresponds to the node type), and a shared pointer to a vector of filtered row indices.
  • loadInternal is the main method to load data into a Velox vector, dispatching based on the node type.
  • populateData is a templated method to populate data for generic types.
  • populateTimestampData is a templated method specialized by clp_s::NodeType to populate timestamp data.

Relevant details from ClpVectorLoader.cpp (full file provided):

  • The file includes usage of clp_s::NodeType enum as a template parameter in populateTimestampData.
  • The loadInternal method uses nodeType_ (of type ColumnType) to decide how to load data.
  • For ColumnType::Timestamp, it checks the dynamic type of columnReader_ against clp_s::Int64ColumnReader, clp_s::DateStringColumnReader, and clp_s::FloatColumnReader and calls populateTimestampData with the corresponding clp_s::NodeType template argument.
  • The populateTimestampData template method is specialized for clp_s::NodeType::Float, Integer, DateString, and Unknown.
  • The file also defines helper functions to convert timestamps from various precisions to Velox Timestamp objects.
  • Explicit template instantiations for populateTimestampData with clp_s::NodeType values are provided at the end of the file.

These snippets and details show how clp_s::NodeType is used as a template parameter to specialize timestamp data loading behavior in ClpVectorLoader. This is directly relevant to understanding the usage of clp_s::NodeType in this file.


✏️ Learnings added
Learnt from: gibber9809
PR: y-scope/velox#15
File: velox/connectors/clp/search_lib/ClpVectorLoader.cpp:22-22
Timestamp: 2025-07-11T15:21:09.634Z
Learning: When analyzing include dependencies, pay special attention to types used in template parameters and enum values like `clp_s::NodeType`, as these may not be immediately obvious in a surface-level review but are essential for compilation.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


#include "velox/connectors/clp/search_lib/ClpVectorLoader.h"
#include "velox/type/Timestamp.h"
#include "velox/vector/ComplexVector.h"
#include "velox/vector/FlatVector.h"

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

namespace {

enum class TimestampPrecision : uint8_t {
Comment thread
gibber9809 marked this conversation as resolved.
Seconds,
Milliseconds,
Microseconds,
Nanoseconds
};

/// Estimates the precision of an epoch timestamp as seconds, milliseconds,
/// microseconds, or nanoseconds.
///
/// This heuristic relies on the fact that 1 year of epoch nanoseconds is
/// approximately 1000 years of epoch microseconds and so on. This heuristic
/// can be unreliable for timestamps sufficiently close to the epoch, but
/// should otherwise be accurate for the next 1000 years.
///
/// Note: Future versions of the clp-s archive format will adopt a
/// nanosecond-precision integer timestamp format (as opposed to the current
/// format which allows other precisions), at which point we can remove this
/// heuristic.
///
/// @param timestamp
/// @return the estimated timestamp precision
template <typename T>
auto estimatePrecision(T timestamp) -> TimestampPrecision {
constexpr int64_t kEpochMilliseconds1971{31536000000};
constexpr int64_t kEpochMicroseconds1971{31536000000000};
constexpr int64_t kEpochNanoseconds1971{31536000000000000};
auto absTimestamp = timestamp >= 0 ? timestamp : -timestamp;

if (absTimestamp > kEpochNanoseconds1971) {
return TimestampPrecision::Nanoseconds;
} else if (absTimestamp > kEpochMicroseconds1971) {
return TimestampPrecision::Microseconds;
} else if (absTimestamp > kEpochMilliseconds1971) {
return TimestampPrecision::Milliseconds;
} else {
return TimestampPrecision::Seconds;
}
}

auto convertToVeloxTimestamp(double timestamp) -> Timestamp {
switch (estimatePrecision(timestamp)) {
case TimestampPrecision::Nanoseconds:
timestamp /= Timestamp::kNanosInSecond;
break;
case TimestampPrecision::Microseconds:
timestamp /= Timestamp::kMicrosecondsInSecond;
break;
case TimestampPrecision::Milliseconds:
timestamp /= Timestamp::kMillisecondsInSecond;
break;
case TimestampPrecision::Seconds:
break;
}
double seconds{std::floor(timestamp)};
double nanoseconds{(timestamp - seconds) * Timestamp::kNanosInSecond};
return Timestamp(
static_cast<int64_t>(seconds), static_cast<uint64_t>(nanoseconds));
}

auto convertToVeloxTimestamp(int64_t timestamp) -> Timestamp {
int64_t precisionDifference{Timestamp::kNanosInSecond};
switch (estimatePrecision(timestamp)) {
case TimestampPrecision::Nanoseconds:
break;
case TimestampPrecision::Microseconds:
precisionDifference =
Timestamp::kNanosInSecond / Timestamp::kNanosecondsInMicrosecond;
break;
case TimestampPrecision::Milliseconds:
precisionDifference =
Timestamp::kNanosInSecond / Timestamp::kNanosecondsInMillisecond;
break;
case TimestampPrecision::Seconds:
precisionDifference =
Timestamp::kNanosInSecond / Timestamp::kNanosInSecond;
break;
}
int64_t seconds{timestamp / precisionDifference};
int64_t nanoseconds{
(timestamp % precisionDifference) *
(Timestamp::kNanosInSecond / precisionDifference)};
if (nanoseconds < 0) {
seconds -= 1;
nanoseconds += Timestamp::kNanosInSecond;
}
return Timestamp(seconds, static_cast<uint64_t>(nanoseconds));
}

} // namespace
Comment thread
gibber9809 marked this conversation as resolved.

ClpVectorLoader::ClpVectorLoader(
clp_s::BaseColumnReader* columnReader,
ColumnType nodeType,
Expand Down Expand Up @@ -57,6 +154,51 @@ void ClpVectorLoader::populateData(RowSet rows, VectorPtr vector) {
}
}

template <clp_s::NodeType Type>
void ClpVectorLoader::populateTimestampData(
RowSet rows,
FlatVector<facebook::velox::Timestamp>* vector) {
bool supportedNodeType{false};
switch (Type) {
case clp_s::NodeType::Float:
case clp_s::NodeType::Integer:
case clp_s::NodeType::DateString:
supportedNodeType = true;
break;
default:
break;
}
if (columnReader_ == nullptr || false == supportedNodeType) {

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)

Simplify boolean comparison.

Use !supportedNodeType instead of false == supportedNodeType for better readability.

-  if (columnReader_ == nullptr || false == supportedNodeType) {
+  if (columnReader_ == nullptr || !supportedNodeType) {
📝 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
if (columnReader_ == nullptr || false == supportedNodeType) {
if (columnReader_ == nullptr || !supportedNodeType) {
🤖 Prompt for AI Agents
In velox/connectors/clp/search_lib/ClpVectorLoader.cpp at line 164, simplify the
boolean comparison by replacing 'false == supportedNodeType' with
'!supportedNodeType' to improve code readability.

for (int vectorIndex : rows) {
vector->setNull(vectorIndex, true);
}
return;
}

for (int vectorIndex : rows) {
auto messageIndex = (*filteredRowIndices_)[vectorIndex];

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::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::DateStringColumnReader*>(columnReader_);
vector->set(
vectorIndex,
convertToVeloxTimestamp(reader->get_encoded_time(messageIndex)));
}
}
}

void ClpVectorLoader::loadInternal(
RowSet rows,
ValueHook* hook,
Expand Down Expand Up @@ -142,6 +284,23 @@ void ClpVectorLoader::loadInternal(
}
break;
}
case ColumnType::Timestamp: {
auto timestampVector = vector->asFlatVector<Timestamp>();
if (nullptr != dynamic_cast<clp_s::Int64ColumnReader*>(columnReader_)) {
populateTimestampData<clp_s::NodeType::Integer>(rows, timestampVector);
} else if (
nullptr !=
dynamic_cast<clp_s::DateStringColumnReader*>(columnReader_)) {
populateTimestampData<clp_s::NodeType::DateString>(
rows, timestampVector);
} else if (
nullptr != dynamic_cast<clp_s::FloatColumnReader*>(columnReader_)) {
populateTimestampData<clp_s::NodeType::Float>(rows, timestampVector);
} else {
populateTimestampData<clp_s::NodeType::Unknown>(rows, timestampVector);
}
Comment on lines +289 to +301

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)

Consider optimizing type detection for better performance.

The current implementation uses multiple dynamic_cast operations in sequence, which could impact performance. Consider storing the column reader type information to avoid runtime type checking.

As a performance optimization, you could:

  1. Store the actual node type in the ClpVectorLoader constructor when the column reader is created
  2. Use a switch statement based on the stored type instead of dynamic casting
  3. This would eliminate the runtime overhead of type checking for each batch of rows
🤖 Prompt for AI Agents
In velox/connectors/clp/search_lib/ClpVectorLoader.cpp around lines 282 to 294,
the code uses multiple dynamic_cast checks to determine the column reader type,
which impacts performance. To fix this, capture and store the column reader's
node type once in the ClpVectorLoader constructor when the column reader is
created. Then replace the dynamic_cast sequence with a switch statement on the
stored node type in this function, calling populateTimestampData accordingly.
This removes repeated runtime type checks and improves efficiency.

break;
}
default:
VELOX_FAIL("Unsupported column type");
}
Expand All @@ -160,5 +319,18 @@ template void ClpVectorLoader::populateData<uint8_t>(
template void ClpVectorLoader::populateData<std::string>(
RowSet rows,
FlatVector<StringView>* vector);
template void ClpVectorLoader::populateTimestampData<clp_s::NodeType::Float>(
RowSet rows,
FlatVector<facebook::velox::Timestamp>* vector);
template void ClpVectorLoader::populateTimestampData<clp_s::NodeType::Integer>(
RowSet rows,
FlatVector<facebook::velox::Timestamp>* vector);
template void
ClpVectorLoader::populateTimestampData<clp_s::NodeType::DateString>(
RowSet rows,
FlatVector<facebook::velox::Timestamp>* vector);
template void ClpVectorLoader::populateTimestampData<clp_s::NodeType::Unknown>(
RowSet rows,
FlatVector<facebook::velox::Timestamp>* vector);

} // namespace facebook::velox::connector::clp::search_lib
10 changes: 10 additions & 0 deletions velox/connectors/clp/search_lib/ClpVectorLoader.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@

#pragma once

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

#include "velox/connectors/clp/search_lib/ClpCursor.h"
#include "velox/type/Timestamp.h"
#include "velox/vector/FlatVector.h"
#include "velox/vector/LazyVector.h"

namespace clp_s {
Expand Down Expand Up @@ -45,6 +50,11 @@ class ClpVectorLoader : public VectorLoader {
template <typename T, typename VectorPtr>
void populateData(RowSet rows, VectorPtr vector);

template <clp_s::NodeType Type>
void populateTimestampData(
RowSet rows,
FlatVector<facebook::velox::Timestamp>* vector);

clp_s::BaseColumnReader* columnReader_;
ColumnType nodeType_;
std::shared_ptr<std::vector<uint64_t>> filteredRowIndices_;
Expand Down
Loading