Skip to content

feat(clp-s)!: Adds CLP+ as an experimental feature. - #2275

Open
davidlion wants to merge 473 commits into
y-scope:mainfrom
davidlion:clpp
Open

davidlion wants to merge 473 commits into
y-scope:mainfrom
davidlion:clpp

Conversation

@davidlion

@davidlion davidlion commented May 12, 2026

Copy link
Copy Markdown
Member

Description

Adds CLP+ (clpp), an experimental extension of clp-s that parses unstructured text logs into structured, query-able fields using log-surgeon. Enabled via --experimental; existing clp-s behaviour is unchanged when the flag is absent.

CLP+ storage & compression

  • CLP+ uses a (log-surgeon parsing spec](https://github.com/y-scope/log-surgeon/blob/log-mechanic/rust/docs/parsing-spec-file.md) to parse the unstructured text during ingestion.
    • Note that for pure unstructured logs, CLP-S requires the log-converter to first change them to JSON.
  • The parsing spec contains rules (named regex patterns) that may contain sub-rules (named regex captures). These patterns split each log message into:
    • Leaf rule matches: a rule match containing no sub-rule. These are stored in CLP-S columns.
    • A log shape: a string containing static text of a log message with the leaf rule matches replaced with %log-surgeon.qualified.leaf_rule_name% placeholders. (Similar to CLP log types.)
    • Parent rule matches: a rule match containing at least one sub-rule. These are stored as log shape metadata, where the parent rule matches for each log shape are "views" into the log shape.
  • The parsing spec is stored in the archive, so extraction and search need only --experimental. Reading a CLP+ archive without it is an error.
  • int/float leaves are encoded as native numeric columns, enabling range queries.
  • Some additional statistics are stored to support special "stat" queries (see below).

Search

  • Extends the KQL grammar with function-call syntax for new filter and and projection functions.
  • Adds the filter function shape(txt) to match against the shape of txt where it is a log message or parent rule field (e.g. shape(msg): "INFO * static log text * %qualified.leaf_rule_name% *").
  • Leaf rules are queryable by their rule path (e.g. message.blockID.genStamp), including wildcards and numeric comparisons.
  • Whole-message and parent-rule value filters depend on query decomposition, which is still under development and not yet reliable. It is gated behind -DCLP_BUILD_CLPP_DECOMPOSITION=ON (default OFF).
  • Adds stats.archives, stats.log_shapes, and stats.schema_tree queries for inspecting archive metadata.

Projection

  • Reworks Projection to track a per-node mode mask (Value/Shape/Decompose) rather than a flat column set, so a single query can request raw text, the shape, and decomposed leaves at different nesting levels.
  • Added two functions that can be run on log message or parent rule fields.
    • shape(txt): output txt's shape as {"txt": {"shape": "..."}}
    • decompose(txt): output txt's shape and all of x's leaf rule fields as {"txt": {"shape": "...", "leaf": [...]}}.

Documentation

  • Adds user docs for CLP+ covering concepts, compression/extraction, search, and projection output.

Build & tests

  • Switches log-surgeon to its Rust implementation and adds a fast_float dependency.
  • Adds integration-tests/tests/binary_tests/test_clpp.py covering round-trip, search, projection, and value encoding; decomposition-dependent tests are skipped unless the build flag is set.
  • Extends the clp-s unit tests to run against both plain and CLP+ archives.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • Verified compression and extraction are lossless by round-tripping the (open-source Hive, Hadoop, OpenStack, and MongoDB datasets](https://docs.yscope.com/clp/main/user-docs/resources-datasets) and comparing the extracted output against clp-s and the original input (made using the log-converter for fully unstructured datasets).
  • Verified search over a compressed archive returns the expected result counts across leaf, shape(), and non-wildcard message and parent rule queries.
    • Wildcard message and parent rule search has know issues and is disabled by default.
  • Verified every projection mode produces the expected JSON structure by comparing against known-good output.
  • Unit tests and the integration test suite pass.

@davidlion davidlion changed the title Clpp feat(clp-s)!: Adds CLP+ as an experimental feature. Sep 11, 2026
@davidlion
davidlion marked this pull request as ready for review September 17, 2026 20:12
@davidlion
davidlion requested a review from a team as a code owner September 17, 2026 20:12

@gibber9809 gibber9809 left a comment

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.

Partial review focusing mostly on the parsing/write side. Should be mostly small changes, though I did spot one potential bug with find_log_shape_id/ClpMatcher that may have slightly bigger scope.

Also took a look at log-converter since I ran into a crash trying to use this branch's version.

* - Forwards `m_archive_writer->update_log_shape_dict`'s return values on failure.
* - Forwards `m_archive_writer->update_parent_rule_shapes`'s return values on failure.
*/
auto parse_str_field(

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.

Minor nit - I think the name of this function might be a little bit too generic, maybe something like parse_log_surgeon_string_field or parse_log_message might be a bit better? (Or at least parse_string_field if you disagree with those).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That is fair, I'm also not happy with that name. Iirc, I used to have something closer to parse_log_message, but I was thinking that really it can parse any string not just log messages. parse_unstructured_string_field (or unstructured_text_field) is probably might be more accurate, but I'm fine with parse_log_message since realistically that is the purpose.

If you have a preference to anything lmk and I'll switch to that.

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 lets go with parse_log_message then.

Comment on lines +137 to +146
/**
* An open `ParentRule` unordered object scope during `parse_str_field`. `match` identifies the
* scope within a `parse_str_field` call. `schema_start` is the scopes starting position in the
* schema and used when closing the unordered object.
*/
struct ParentScope {
log_surgeon::Match const* match;
size_t schema_start;
SchemaNode::id_t tree_node_id;
};

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.

I think that the schema_start name is mechanically accurate, but maybe not the clearest terminology since it mixes mechanism with functionality.

Instead of schema_start, maybe something like scope_handle, schema_handle, or rule_handle would make the functionality more clear? schema_start is aligned more with how we've been naming this in the rest of the code, but I don't think our existing naming is that good for this feature.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ya I don't like the name either (arguably it isn't super accurate either because that index isn't the "start of a schema" it is the scope's start within the schema).

I feel like I always associate "handle" with something that wraps a resource/object. I don't think I've ever used it for an index. What do you think of scope_schema_idx, scope_start_schema_idx. I got pretty confused working with schema entries so I personally would prefer there be some sort of relationship. At one point I was trying to refactor this stuff even more, but it was taking too long.

If you feel strongly about any name, I'll go with what you prefer.

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.

I think scope_schema_idx sounds good to me.

Comment on lines +195 to +199
auto convert_string_to_double(std::string_view raw, double& converted) -> bool {
auto const res{fast_float::from_chars(raw.begin(), raw.end(), converted)};
return res.ptr == raw.end() && std::errc{} == res.ec;
}

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.

Beyond the scope of this PR, but I'm making a note to check if this fast_float library can help speed up the restore_encoded_float utility from clp-s.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think that is worthwhile. Fwiw, the reason I left fast_float in is because I did notice a non-trivial improvement when the encoding was still buggy (and trying to convert everything).

log_msg_node_id,
*m_archive_writer
)};
SchemaNode::id_t node_id{0};

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.

Suggested change
SchemaNode::id_t node_id{0};
SchemaNode::id_t node_id{-1};

Should probably use -1 or std::optional<SchemaNode::id_t>, just because 0 is a valid node ID, even if by construction the leaf node here should never be node 0.

Comment on lines +57 to +58
[[nodiscard]] auto get_next_id() const -> uint64_t { return m_next_id; }

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.

Suggested change
[[nodiscard]] auto get_next_id() const -> uint64_t { return m_next_id; }

Not used, as far as I can tell?

Comment on lines +140 to +141
// The closing placeholder delimiter is always the next '%' after the opening one as a
// column name cannot contain a delimiter.

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.

Is this (making sure rule names can't contain %) enforced when parsing the spec file in log-surgeon right now?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's correct. Although, now I realize it isn't explicitly stated in the log-surgeon docs. Iirc the only symbol is _.

Comment on lines +143 to +146
if (std::string_view::npos == close) {
result.emplace_back(Segment::Type::Literal, shape.substr(open));
return result;
}

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.

Can we hit this case for a valid text shape?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, good point. Do you think throwing is the best option since this basically means a bug occurred or there is corruption? Or what do you think is the best option?

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 I think throw/error is probably the right thing to do.

Comment on lines +15 to +20
compressor.write_numeric_value(m_parent_rule_shapes.size());
for (auto const& shape : m_parent_rule_shapes) {
compressor.write_numeric_value(shape.m_name.size());
compressor.write_string(shape.m_name);
compressor.write_numeric_value(shape.m_start);
compressor.write_numeric_value(shape.m_size);

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.

Suggested change
compressor.write_numeric_value(m_parent_rule_shapes.size());
for (auto const& shape : m_parent_rule_shapes) {
compressor.write_numeric_value(shape.m_name.size());
compressor.write_string(shape.m_name);
compressor.write_numeric_value(shape.m_start);
compressor.write_numeric_value(shape.m_size);
compressor.write_numeric_value<uint64_t>(m_parent_rule_shapes.size());
for (auto const& shape : m_parent_rule_shapes) {
compressor.write_numeric_value<uint64_t>(shape.m_name.size());
compressor.write_string(shape.m_name);
compressor.write_numeric_value<uint64_t>(shape.m_start);
compressor.write_numeric_value<uint64_t>(shape.m_size);

Just because if we compile this for a 32-bit target (like wasm) we can accidentally change the archive format if we're writing/reading size_t directly. Could also choose uint32_t or something here, just need to be explicit with whatever we choose here + on the read side.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Does this make sense to you:

If 32-bit targets are a legitimate case, we probably want to explicitly limit everything to uint32_. Otherwise, I think we'd need to explicit write the size of size_t of the archive writer (for the reader to use).

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.

Right now all of the field sizes are explicitly defined in the format (there's a lot of 64-bit fields that really don't need to be 64-bit, but we can't change them because of backwards compatibility). When we added support for wasm what we did was we went through all of the 64-bit fields in the archive format that need to get treated as a size_t in the code after decompression and added an extra check to ensure that the uint64 in the archive could be converted to size_t on the target platform (using the ReaderUtils::try_uint64_to_size_t utility). Basically the 32-bit wasm path handles this problem by explicitly error-ing out on some archives.

But yeah, since for legitimate use-cases these values should fit within a uint32_t anyway I'd be fine with just using uint32_t in the format for all the places I pointed out in my review and making our lives easier.

namespace clpp {
auto LogShapeStat::compress(clp_s::ZstdCompressor& compressor) const
-> ystdlib::error_handling::Result<void> {
compressor.write_numeric_value(m_count);

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.

Suggested change
compressor.write_numeric_value(m_count);
compressor.write_numeric_value<uint64_t>(m_count);

Same as my other comment -- should update the read and write side to use some explicit integer type besides size_t.

template <typename Element, typename Index>
auto Array<Element, Index>::compress(clp_s::ZstdCompressor& compressor)
-> ystdlib::error_handling::Result<void> {
compressor.write_numeric_value(m_array.size());

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.

Suggested change
compressor.write_numeric_value(m_array.size());
compressor.write_numeric_value<uint64_t>(m_array.size());

Same as other comments -- need to use explicit integer type for read/write side to avoid issue with size_t changing size between 32/64-bit platforms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants