feat: Implement support for marshalling columns to velox Timestamp type. - #15
Conversation
WalkthroughSupport for the Changes
Sequence Diagram(s)sequenceDiagram
participant QueryEngine
participant ClpDataSource
participant ClpCursor
participant ClpVectorLoader
QueryEngine->>ClpDataSource: Request data (includes TIMESTAMP columns)
ClpDataSource->>ClpCursor: Map Velox TIMESTAMP to ColumnType::Timestamp
ClpCursor->>ClpVectorLoader: Request vector loading for ColumnType::Timestamp
ClpVectorLoader->>ClpVectorLoader: Detect timestamp precision, convert raw values
ClpVectorLoader-->>ClpCursor: Return populated Timestamp vector
ClpCursor-->>ClpDataSource: Return loaded data
ClpDataSource-->>QueryEngine: Return data with TIMESTAMP columns
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)velox/connectors/clp/search_lib/ClpVectorLoader.cpp (2)🧬 Code Graph Analysis (1)velox/connectors/clp/search_lib/ClpVectorLoader.cpp (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
…ompressed using --timestamp-key timestamp.
There was a problem hiding this comment.
Why'd this change? Did the file it was generated from change?
There was a problem hiding this comment.
This one was compressed without the --timestamp-key timestamp flag previously. I've updated it so that "timestamp" is now a DateString in the archive instead of a VarString. The test_1.clps archive already seems to have been compressed with --timestamp-key timestamp.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
velox/connectors/clp/ClpDataSource.cpp(1 hunks)velox/connectors/clp/search_lib/ClpCursor.cpp(1 hunks)velox/connectors/clp/search_lib/ClpCursor.h(1 hunks)velox/connectors/clp/search_lib/ClpVectorLoader.cpp(4 hunks)velox/connectors/clp/search_lib/ClpVectorLoader.h(2 hunks)velox/connectors/clp/tests/ClpConnectorTest.cpp(10 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
velox/connectors/clp/tests/ClpConnectorTest.cpp (1)
velox/serializers/tests/PrestoSerializerTest.cpp (1)
makeRowVector(804-821)
velox/connectors/clp/search_lib/ClpVectorLoader.cpp (1)
velox/connectors/clp/search_lib/ClpVectorLoader.h (2)
populateTimestampData(54-56)rows(44-48)
🔇 Additional comments (5)
velox/connectors/clp/search_lib/ClpCursor.h (1)
52-60: LGTM! Clean enum extension for timestamp support.The
Timestampenumerator is correctly added to theColumnTypeenum with proper placement and formatting.velox/connectors/clp/ClpDataSource.cpp (1)
96-98: LGTM! Correct type mapping for timestamp support.The mapping from
TypeKind::TIMESTAMPtoColumnType::Timestampis properly implemented and follows the established pattern.velox/connectors/clp/search_lib/ClpVectorLoader.h (1)
19-21: LGTM! Well-structured additions for timestamp vector loading.The new includes are appropriate for timestamp support, and the templated
populateTimestampDatamethod follows established patterns in the class design.Also applies to: 23-24, 53-56
velox/connectors/clp/tests/ClpConnectorTest.cpp (2)
39-41: LGTM! Clear constants for test timestamp values.The constants provide a clear reference for the test timestamp "2025-04-30T08:50:05Z" used across multiple test cases.
174-174: LGTM! Comprehensive test updates for timestamp support.All relevant test cases have been properly updated to use
TIMESTAMP()type instead ofVARCHAR()and the expected results correctly use VeloxTimestampobjects. The test updates are consistent across all affected test methods.Also applies to: 182-182, 199-211, 221-221, 234-234, 247-259, 269-269, 281-281, 297-309
| 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); | ||
| } |
There was a problem hiding this comment.
🧹 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:
- Store the actual node type in the
ClpVectorLoaderconstructor when the column reader is created - Use a switch statement based on the stored type instead of dynamic casting
- 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.
| default: | ||
| break; | ||
| } | ||
| if (columnReader_ == nullptr || false == supportedNodeType) { |
There was a problem hiding this comment.
🧹 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.
| 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.
| #include <utility> | ||
|
|
||
| #include "clp_s/ColumnReader.hpp" | ||
| #include "clp_s/SchemaTree.hpp" |
There was a problem hiding this comment.
🧹 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.
| #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.
There was a problem hiding this comment.
It is used for clp_s::NodeType
There was a problem hiding this comment.
🧩 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, aColumnType(which corresponds to the node type), and a shared pointer to a vector of filtered row indices. loadInternalis the main method to load data into a Velox vector, dispatching based on the node type.populateDatais a templated method to populate data for generic types.populateTimestampDatais a templated method specialized byclp_s::NodeTypeto populate timestamp data.
Relevant details from ClpVectorLoader.cpp (full file provided):
- The file includes usage of
clp_s::NodeTypeenum as a template parameter inpopulateTimestampData. - The
loadInternalmethod usesnodeType_(of typeColumnType) to decide how to load data. - For
ColumnType::Timestamp, it checks the dynamic type ofcolumnReader_againstclp_s::Int64ColumnReader,clp_s::DateStringColumnReader, andclp_s::FloatColumnReaderand callspopulateTimestampDatawith the correspondingclp_s::NodeTypetemplate argument. - The
populateTimestampDatatemplate method is specialized forclp_s::NodeType::Float,Integer,DateString, andUnknown. - The file also defines helper functions to convert timestamps from various precisions to Velox
Timestampobjects. - Explicit template instantiations for
populateTimestampDatawithclp_s::NodeTypevalues 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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
velox/connectors/clp/search_lib/ClpCursor.cpp (1)
216-220: Add missing break statement to prevent fall-through.The new
ColumnType::Timestampcase is missing abreakstatement, which will cause execution to fall through to thedefaultcase.case ColumnType::Timestamp: columnDescriptor->set_matching_types( LiteralType::EpochDateT | LiteralType::IntegerT | LiteralType::FloatT); + break;velox/connectors/clp/search_lib/ClpVectorLoader.cpp (5)
53-56: Fix incorrect nanoseconds constant.The
kEpochNanoseconds1971constant has the wrong value. It should be31536000000000000(17 digits) instead of31536000000000(14 digits, same as microseconds).- constexpr int64_t kEpochNanoseconds1971{31536000000000}; + constexpr int64_t kEpochNanoseconds1971{31536000000000000};
87-87: Use std::floor for consistency.For consistency with other standard library functions, use
std::floorinstead offloor.- double seconds{floor(timestamp)}; + double seconds{std::floor(timestamp)};
22-22: Remove unused include.The include
"clp_s/SchemaTree.hpp"appears to be unused in this file.-#include "clp_s/SchemaTree.hpp"
164-164: Simplify boolean comparison.Use
!supportedNodeTypeinstead offalse == supportedNodeTypefor better readability.- if (columnReader_ == nullptr || false == supportedNodeType) { + if (columnReader_ == nullptr || !supportedNodeType) {
282-294: Consider optimizing type detection for better performance.The current implementation uses multiple
dynamic_castoperations in sequence, which could impact performance. Consider storing the column reader type information to avoid runtime type checking.As a performance optimization, you could:
- Store the actual node type in the
ClpVectorLoaderconstructor when the column reader is created- Use a switch statement based on the stored type instead of dynamic casting
- This would eliminate the runtime overhead of type checking for each batch of rows
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
velox/connectors/clp/search_lib/ClpCursor.cpp(1 hunks)velox/connectors/clp/search_lib/ClpVectorLoader.cpp(4 hunks)velox/connectors/clp/tests/ClpConnectorTest.cpp(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (1)
velox/connectors/clp/tests/ClpConnectorTest.cpp (1)
29-42: Test changes properly validate timestamp functionality.The test updates correctly change column types from
VARCHARtoTIMESTAMPand use properTimestampobjects with the defined constants. The epoch seconds value (1746003005) accurately represents "2025-04-30T08:50:05Z".Also applies to: 173-175, 220-221, 233-234, 268-269, 280-281, 297-298
Co-authored-by: Xiaochong(Eddy) Wei <40865608+anlowee@users.noreply.github.com>
| auto convertToVeloxTimestamp(int64_t timestamp) -> Timestamp { | ||
| switch (estimatePrecision(timestamp)) { | ||
| case TimestampPrecision::Nanoseconds: | ||
| break; | ||
| case TimestampPrecision::Microseconds: | ||
| timestamp *= Timestamp::kNanosecondsInMicrosecond; | ||
| break; | ||
| case TimestampPrecision::Milliseconds: | ||
| timestamp *= Timestamp::kNanosecondsInMillisecond; | ||
| break; | ||
| case TimestampPrecision::Seconds: | ||
| timestamp *= Timestamp::kNanosInSecond; | ||
| break; | ||
| } | ||
| int64_t seconds{timestamp / Timestamp::kNanosInSecond}; | ||
| int64_t nanoseconds{timestamp - seconds * Timestamp::kNanosInSecond}; | ||
| if (nanoseconds < 0) { | ||
| seconds -= 1; | ||
| nanoseconds += Timestamp::kNanosInSecond; | ||
| } | ||
| return Timestamp(seconds, static_cast<uint64_t>(nanoseconds)); | ||
| } |
There was a problem hiding this comment.
| auto convertToVeloxTimestamp(int64_t timestamp) -> Timestamp { | |
| switch (estimatePrecision(timestamp)) { | |
| case TimestampPrecision::Nanoseconds: | |
| break; | |
| case TimestampPrecision::Microseconds: | |
| timestamp *= Timestamp::kNanosecondsInMicrosecond; | |
| break; | |
| case TimestampPrecision::Milliseconds: | |
| timestamp *= Timestamp::kNanosecondsInMillisecond; | |
| break; | |
| case TimestampPrecision::Seconds: | |
| timestamp *= Timestamp::kNanosInSecond; | |
| break; | |
| } | |
| int64_t seconds{timestamp / Timestamp::kNanosInSecond}; | |
| int64_t nanoseconds{timestamp - seconds * Timestamp::kNanosInSecond}; | |
| if (nanoseconds < 0) { | |
| seconds -= 1; | |
| nanoseconds += Timestamp::kNanosInSecond; | |
| } | |
| return Timestamp(seconds, static_cast<uint64_t>(nanoseconds)); | |
| } | |
| auto convertToVeloxTimestamp(int64_t timestamp) -> Timestamp { | |
| int64_t seconds{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; | |
| } | |
| seconds /= precisionDifference; | |
| int64_t nanoseconds = timestamp % Timestamp::kNanosInSecond; | |
| if (nanoseconds < 0) { | |
| seconds -= 1; | |
| nanoseconds += Timestamp::kNanosInSecond; | |
| } | |
| return Timestamp(seconds, static_cast<uint64_t>(nanoseconds)); | |
| } |
A possible way to avoid overflow permenantly
There was a problem hiding this comment.
I think we would need to do
int64_t nanoseconds{timestamp % precisionDifference * (Timestamp::kNanosInSecond / precisionDifference)};
but yeah, this should work.
Should we update the implementation then, or is it fine to leave it?
There was a problem hiding this comment.
it would be good if we can update it in this PR, this would be the final comment for this PR
…ger-overflow (facebookincubator#13831) Summary: Pull Request resolved: facebookincubator#13831 This avoids the following errors: ``` fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/std_abs.h:56:41: runtime error: negation of -9223372036854775808 cannot be represented in type 'long'; cast to an unsigned type to negate this value to itself #0 0x000000346ce5 in std::abs(long) fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/std_abs.h:56 #1 0x000000345879 in std::shared_ptr<facebook::velox::BiasVector<facebook::velox::test::EvalTypeHelper<long>::Type>> facebook::velox::test::VectorMaker::biasVector<long>(std::vector<std::optional<long>, std::allocator<std::optional<long>>> const&) fbcode/velox/vector/tests/utils/VectorMaker-inl.h:58 #2 0x000000344d34 in facebook::velox::test::BiasVectorErrorTest::errorTest(std::vector<std::optional<long>, std::allocator<std::optional<long>>>) fbcode/velox/vector/tests/BiasVectorTest.cpp:39 #3 0x00000033ec99 in facebook::velox::test::BiasVectorErrorTest_checkRangeTooLargeError_Test::TestBody() fbcode/velox/vector/tests/BiasVectorTest.cpp:44 #4 0x7fe0a2342c46 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) fbsource/src/gtest.cc:2727 #5 0x7fe0a234275d in testing::Test::Run() fbsource/src/gtest.cc:2744 #6 0x7fe0a2345fb3 in testing::TestInfo::Run() fbsource/src/gtest.cc:2890 #7 0x7fe0a234c8eb in testing::TestSuite::Run() fbsource/src/gtest.cc:3068 #8 0x7fe0a237b52b in testing::internal::UnitTestImpl::RunAllTests() fbsource/src/gtest.cc:6059 #9 0x7fe0a237a0a2 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) fbsource/src/gtest.cc:2727 #10 0x7fe0a23797f5 in testing::UnitTest::Run() fbsource/src/gtest.cc:5599 #11 0x7fe0a2239800 in RUN_ALL_TESTS() fbsource/gtest/gtest.h:2334 #12 0x7fe0a223952c in main fbcode/common/gtest/LightMain.cpp:20 #13 0x7fe09ec2c656 in __libc_start_call_main /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../sysdeps/nptl/libc_start_call_main.h:58:16 #14 0x7fe09ec2c717 in __libc_start_main@GLIBC_2.2.5 /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../csu/libc-start.c:409:3 #15 0x00000033d8b0 in _start /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../sysdeps/x86_64/start.S:116 UndefinedBehaviorSanitizer: signed-integer-overflow fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/std_abs.h:56:41 ``` Avoid overflow by using the expression (static_cast<uint64_t>(1) + ~static_cast<uint64_t>(min)) to calculate the absolute value of min without using std::abs Reviewed By: dmm-fb, peterenescu Differential Revision: D76901449 fbshipit-source-id: 7eb3bd0f83e42f44cdf34ea1759f3aa9e1042dae
3 participants
Description
This PR adds support for marshalling timestamps in clp-s to the velox Timestamp type. Since clp-s doesn't have a standard timestamp representation (timestamps can currently be strings, floats, ints, and have different time precisions), we use some heuristics to convert the various timestamp formats to nanosecond precision.
We should be able to eliminate the heuristics in a future PR once we move to a standard nanosecond precision integer (likely delta-encoded) timestamp representation in a future version of the clp-s archive format.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Summary by CodeRabbit