Skip to content

fix(clp-s): Handle pure wildcards and unexpected literal types correctly in EvaluateTimestampIndex (fixes #1096). #1277

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gibber9809 merged 11 commits into from
Sep 3, 2025
Merged

fix(clp-s): Handle pure wildcards and unexpected literal types correctly in EvaluateTimestampIndex (fixes #1096). #1277

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6b0d3af
Handle pure wildcards and unexpected literal types correctly in
gibber9809 Aug 29, 2025
ba7d320
Add timestamp key option to clp_s_test_utils
gibber9809 Aug 29, 2025
8056b96
Add test that exercises crashes from issue 1096
gibber9809 Aug 29, 2025
207b10d
Address rabbit comments
gibber9809 Aug 29, 2025
39d3e63
Merge branch 'main' into fix-1096
kirkrodrigues Sep 1, 2025
bac07e5
Use some rabbit suggestions
gibber9809 Sep 2, 2025
00d5fff
Address another rabbit comment
gibber9809 Sep 2, 2025
d957968
Minor fix
gibber9809 Sep 2, 2025
0467291
Remove redundant special cases in EvaluateTimestampIndex
gibber9809 Sep 2, 2025
f33b147
Merge remote-tracking branch 'upstream/main' into fix-1096
gibber9809 Sep 2, 2025
e897bf1
Merge branch 'main' into fix-1096
gibber9809 Sep 3, 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
57 changes: 41 additions & 16 deletions components/core/src/clp_s/search/EvaluateTimestampIndex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@
#include "ast/AndExpr.hpp"
#include "ast/Expression.hpp"
#include "ast/FilterExpr.hpp"
#include "ast/FilterOperation.hpp"
#include "ast/Integral.hpp"
#include "ast/Literal.hpp"
#include "ast/OrExpr.hpp"

using clp_s::search::ast::AndExpr;
using clp_s::search::ast::Expression;
using clp_s::search::ast::FilterExpr;
using clp_s::search::ast::FilterOperation;
using clp_s::search::ast::Integral;
using clp_s::search::ast::Integral64;
using clp_s::search::ast::literal_type_bitmask_t;
Expand Down Expand Up @@ -63,16 +65,19 @@ EvaluatedValue EvaluateTimestampIndex::run(std::shared_ptr<Expression> const& ex
range_it != m_timestamp_dict->tokenized_column_to_range_end();
range_it++)
{
// Don't attempt to evaluate the timestamp index against columns with wildcard tokens.
if (column->is_unresolved_descriptor()) {
continue;
}

std::vector<std::string> const& tokens = range_it->first;
auto const& descriptors = column->get_descriptor_list();
// TODO: handle wildcard matching; the initial check on timestamp index happens
// before schema matching, so
if (tokens.size() != descriptors.size()) {
continue;
}

bool matched = true;
for (size_t i = 0; i < descriptors.size(); ++i) {
for (size_t i{0ULL}; i < descriptors.size(); ++i) {
Comment thread
gibber9809 marked this conversation as resolved.
Resolved
if (tokens[i] != descriptors[i].get_token()) {
matched = false;
break;
Expand All @@ -82,20 +87,40 @@ EvaluatedValue EvaluateTimestampIndex::run(std::shared_ptr<Expression> const& ex
continue;
}

EvaluatedValue ret;
// this is safe after type narrowing because all DateType literals are either
// Integral or a derived class of Integral
Integral64 literal = std::static_pointer_cast<Integral>(filter->get_operand())->get();
if (std::holds_alternative<int64_t>(literal)) {
ret = range_it->second->evaluate_filter(
filter->get_operation(),
std::get<int64_t>(literal)
);
// Leave handling EXISTS/NEXISTS to schema matching.
auto const filter_op{filter->get_operation()};
if (FilterOperation::EXISTS == filter_op || FilterOperation::NEXISTS == filter_op) {
return EvaluatedValue::Unknown;

@wraymo wraymo Sep 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correct me if I'm wrong. ts: * is an existence check and we convert it from EQ to EXISTS before evaluating the timestamp filter. Why do we return Unknown here even when we find a match?

@gibber9809 gibber9809 Sep 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is just evaluating against the timestamp index -- it won't tell us whether the timestamp exists on a given record/schema, so we leave that up to the schema matching pass.

@wraymo wraymo Sep 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Quick question: what happens if an archive contains some messages without timestamp fields? I think we simply skip recording timestamps for those messages? But if a query includes timestamp filters and none of the timestamped messages in that archive match (out of range), should we still scan the remaining messages that don’t have timestamp fields?

@gibber9809 gibber9809 Sep 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah we currently don't record any timestamps and will skip them in a scan that contains a timestamp filter. It probably makes sense to add some options for users to get around this (e.g. assign timestamp based on surrounding log events at ingestion time, or potentially still search log events missing timestamps if the timestamp range index matches), but that's future work.

}

@coderabbitai coderabbitai Bot Aug 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Hoist EXISTS/NEXISTS guard before the range loop.

This result is invariant of the timestamp dictionary; return Unknown earlier to skip unnecessary work.

Apply:

@@
-        for (auto range_it = m_timestamp_dict->tokenized_column_to_range_begin();
+        // Leave handling EXISTS/NEXISTS to schema matching.
+        auto const filter_op{filter->get_operation()};
+        if (FilterOperation::EXISTS == filter_op || FilterOperation::NEXISTS == filter_op) {
+            return EvaluatedValue::Unknown;
+        }
+
+        for (auto range_it = m_timestamp_dict->tokenized_column_to_range_begin();
              range_it != m_timestamp_dict->tokenized_column_to_range_end();
              range_it++)
         {
@@
-            // Leave handling EXISTS/NEXISTS to schema matching.
-            auto const filter_op{filter->get_operation()};
-            if (FilterOperation::EXISTS == filter_op || FilterOperation::NEXISTS == filter_op) {
-                return EvaluatedValue::Unknown;
-            }
📝 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
auto const filter_op{filter->get_operation()};
if (FilterOperation::EXISTS == filter_op || FilterOperation::NEXISTS == filter_op) {
return EvaluatedValue::Unknown;
}
// Leave handling EXISTS/NEXISTS to schema matching.
auto const filter_op{filter->get_operation()};
if (FilterOperation::EXISTS == filter_op || FilterOperation::NEXISTS == filter_op) {
return EvaluatedValue::Unknown;
}
for (auto range_it = m_timestamp_dict->tokenized_column_to_range_begin();
range_it != m_timestamp_dict->tokenized_column_to_range_end();
range_it++)
{
// ... rest of loop body (no longer needs the EXISTS/NEXISTS check here)
}
🤖 Prompt for AI Agents 
In components/core/src/clp_s/search/EvaluateTimestampIndex.cpp around lines 88
to 91, the EXISTS/NEXISTS check is performed inside or after a loop over the
timestamp dictionary; hoist this guard so it executes before entering the range
loop. Change flow so that if filter->get_operation() is FilterOperation::EXISTS
or FilterOperation::NEXISTS the function immediately returns
EvaluatedValue::Unknown, avoiding any iteration or dictionary access; ensure no
other logic relies on side-effects from the loop before this early return.


auto const literal{filter->get_operand()};
// Defend against future FilterOperation types that don't take a literal.
if (nullptr == literal) {
return EvaluatedValue::Unknown;
}

@coderabbitai coderabbitai Bot Aug 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Hoist null-literal guard before the range loop as well.

Like the EXISTS/NEXISTS case, this is dictionary-independent.

Apply:

@@
-        for (auto range_it = m_timestamp_dict->tokenized_column_to_range_begin();
+        auto const literal{filter->get_operand()};
+        // Defend against FilterOperation types that don't take a literal.
+        if (nullptr == literal) {
+            return EvaluatedValue::Unknown;
+        }
+
+        for (auto range_it = m_timestamp_dict->tokenized_column_to_range_begin();
              range_it != m_timestamp_dict->tokenized_column_to_range_end();
              range_it++)
         {
@@
-            auto const literal{filter->get_operand()};
-            // Defend against future FilterOperation types that don't take a literal.
-            if (nullptr == literal) {
-                return EvaluatedValue::Unknown;
-            }
📝 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
auto const literal{filter->get_operand()};
// Defend against future FilterOperation types that don't take a literal.
if (nullptr == literal) {
return EvaluatedValue::Unknown;
}
// Hoist the null-literal guard out of the loop, since it doesn’t depend on the dictionary.
auto const literal{filter->get_operand()};
// Defend against FilterOperation types that don't take a literal.
if (nullptr == literal) {
return EvaluatedValue::Unknown;
}
for (auto range_it = m_timestamp_dict->tokenized_column_to_range_begin();
range_it != m_timestamp_dict->tokenized_column_to_range_end();
range_it++)
{
- auto const literal{filter->get_operand()};
- // Defend against future FilterOperation types that don't take a literal.
- if (nullptr == literal) {
- return EvaluatedValue::Unknown;
- }
// ... rest of loop body using `literal` ...
}
🤖 Prompt for AI Agents 
In components/core/src/clp_s/search/EvaluateTimestampIndex.cpp around lines 93
to 97, the null-literal guard is currently inside the range-handling path; hoist
that guard before entering the range loop so we defend against future
FilterOperation types irrespective of dictionary logic. Move the nullptr ==
literal check to immediately after obtaining literal (before any loop or range
processing) and early-return EvaluatedValue::Unknown if literal is null,
ensuring both the EXISTS/NEXISTS and range cases use the same
dictionary-independent guard.


// Handle the case where the literal is a pure wildcard.
if (literal->as_any(filter_op)) {

@wraymo wraymo Sep 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And do we have still have an EQ case here if we it's already converted to EXISTS?

@gibber9809 gibber9809 Sep 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't if we run this after the convert to exists pass. Thinking about this more though we should also return Unknown here and just leave it up to schema matching for the same reason I mention in my other comment.

Since that's the case I think I can just delete this code block and rely on following through to the Unknown case below.

@gibber9809 gibber9809 Sep 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually I can probably also delete the special case dedicated to exists/nexists and rely on the check for whether the filter has a literal -- going to do a pass and get rid of redundant special cases.

@wraymo wraymo Sep 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, that's cool

switch (filter_op) {
case FilterOperation::EQ:
return filter->is_inverted() ? EvaluatedValue::False : EvaluatedValue::True;
case FilterOperation::NEQ:
return filter->is_inverted() ? EvaluatedValue::True : EvaluatedValue::False;
default:
// Not reachable.
return EvaluatedValue::Unknown;
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

EvaluatedValue ret{EvaluatedValue::Unknown};
int64_t int_literal{};
double float_literal{};
if (literal->as_int(int_literal, filter_op)) {
ret = range_it->second->evaluate_filter(filter_op, int_literal);
} else if (literal->as_float(float_literal, filter_op)) {
ret = range_it->second->evaluate_filter(filter_op, float_literal);
} else {
ret = range_it->second->evaluate_filter(
filter->get_operation(),
std::get<double>(literal)
);
return EvaluatedValue::Unknown;
}

if (ret == EvaluatedValue::True) {
Expand Down
5 changes: 5 additions & 0 deletions components/core/tests/clp_s_test_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "clp_s_test_utils.hpp"

#include <filesystem>
#include <optional>
#include <string>
#include <vector>

Expand All @@ -13,6 +14,7 @@
auto compress_archive(
std::string const& file_path,
std::string const& archive_directory,
std::optional<std::string> timestamp_key,
bool single_file_archive,
bool structurize_arrays,
clp_s::FileType file_type
Expand All @@ -39,6 +41,9 @@ auto compress_archive(
parser_option.structurize_arrays = structurize_arrays;
parser_option.single_file_archive = single_file_archive;
parser_option.input_file_type = file_type;
if (timestamp_key.has_value()) {
parser_option.timestamp_key = std::move(timestamp_key.value());
}
Comment thread
gibber9809 marked this conversation as resolved.
Resolved

clp_s::JsonParser parser{parser_option};
std::vector<clp_s::ArchiveStats> archive_stats;
Expand Down
2 changes: 2 additions & 0 deletions components/core/tests/clp_s_test_utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#ifndef CLP_S_TEST_UTILS_HPP
#define CLP_S_TEST_UTILS_HPP

#include <optional>
#include <string>
#include <vector>

Expand All @@ -22,6 +23,7 @@
[[nodiscard]] auto compress_archive(
std::string const& file_path,
std::string const& archive_directory,
std::optional<std::string> timestamp_key,
bool single_file_archive,
bool structurize_arrays,
clp_s::FileType file_type
Expand Down
2 changes: 2 additions & 0 deletions components/core/tests/test-clp_s-delta-encode-log-order.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include <cstdint>
#include <filesystem>
#include <memory>
#include <optional>
#include <string>
#include <string_view>
#include <vector>
Expand Down Expand Up @@ -68,6 +69,7 @@ TEST_CASE("clp-s-delta-encode-log-order", "[clp-s][delta-encode-log-order]") {
REQUIRE_NOTHROW(compress_archive(
get_test_input_local_path(),
std::string{cTestDeltaEncodeOrderArchiveDirectory},
std::nullopt,
true,
false,
clp_s::FileType::Json
Expand Down
2 changes: 2 additions & 0 deletions components/core/tests/test-clp_s-end_to_end.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <cstdlib>
#include <filesystem>
#include <optional>
#include <string>
#include <string_view>
#include <utility>
Expand Down Expand Up @@ -108,6 +109,7 @@ TEST_CASE("clp-s-compress-extract-no-floats", "[clp-s][end-to-end]") {
std::ignore = compress_archive(
get_test_input_local_path(),
std::string{cTestEndToEndArchiveDirectory},
std::nullopt,
single_file_archive,
structurize_arrays,
clp_s::FileType::Json
Expand Down
2 changes: 2 additions & 0 deletions components/core/tests/test-clp_s-range_index.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <cstdlib>
#include <filesystem>
#include <optional>
#include <string>
#include <string_view>

Expand Down Expand Up @@ -224,6 +225,7 @@ TEST_CASE("clp-s-range-index", "[clp-s][range-index]") {
archive_stats = compress_archive(
input_file,
std::string{cTestRangeIndexArchiveDirectory},
std::nullopt,
single_file_archive,
false,
input_file_type
Expand Down
5 changes: 4 additions & 1 deletion components/core/tests/test-clp_s-search.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include <exception>
#include <filesystem>
#include <memory>
#include <optional>
#include <set>
#include <sstream>
#include <string>
Expand Down Expand Up @@ -223,7 +224,8 @@ TEST_CASE("clp-s-search", "[clp-s][search]") {
R"aa(idx: 0 OR idx: 1)aa",
{1}},
{R"aa(ambiguous_varstring: "a*e")aa", {10, 11, 12}},
{R"aa(ambiguous_varstring: "a\*e")aa", {12}}
{R"aa(ambiguous_varstring: "a\*e")aa", {12}},
{R"aa(idx: * AND NOT idx: null AND idx: 0)aa", {0}}
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
auto structurize_arrays = GENERATE(true, false);
auto single_file_archive = GENERATE(true, false);
Expand All @@ -234,6 +236,7 @@ TEST_CASE("clp-s-search", "[clp-s][search]") {
std::ignore = compress_archive(
get_test_input_local_path(),
std::string{cTestSearchArchiveDirectory},
std::string{cTestIdxKey},
single_file_archive,
structurize_arrays,
clp_s::FileType::Json
Expand Down