Clpp by davidlion · Pull Request #2275 · y-scope/clp

davidlion · 2026-05-12T03:58:06Z

Description

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

…ackage Docker Compose project (resolves y-scope#1747). (y-scope#1773) Co-authored-by: sitaowang1998 <sitaowang1998@outlook.com>

…ngs to server configuration (fixes y-scope#1799). (y-scope#1803) Co-authored-by: davemarco <83603688+davemarco@users.noreply.github.com>

…ation for new schema (fixes y-scope#1866). (y-scope#1867)

…efault when value is empty (fixes y-scope#1854). (y-scope#1855)

…type inference (fixes y-scope#1864). (y-scope#1865) Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>

…uling. (y-scope#1829) Co-authored-by: Devin Gibson <gibber9809@users.noreply.github.com> Co-authored-by: Kirk Rodrigues <2454684+kirkrodrigues@users.noreply.github.com>

…e mutually exclusive in sbin search scripts (fixes y-scope#1859). (y-scope#1871)

…ctoring.

… match.

…ist (hopfully not possible).

…_view.

…ls due to parsing the next event's timestamp.

…tion tests

Changed create_output_handler refactor.

gibber9809 · 2026-06-29T15:41:53Z

-            m_string_type |= LiteralType::ClpStringT;
-        }
+        // if (m_v.find(' ') != std::string::npos) {
+        m_string_type = LiteralType::ClpStringT | LiteralType::VarStringT;


Making a note that we'll need to resolve this properly before merging.

I think one thing we could do is just handle column resolution in a special way for clp+ that understands that the VarString/ClpString types are interchangeable for its purposes, and also just add a new as_string(...) method to Literal.hpp that gives you a string if the underlying is either VarString or ClpString.

The main complicating factors are:

Will have to watch out for any filtering code that uses as_var_string/as_clp_string

The NarrowTypes pass handles some special cases for not equals queries on predicates that can be interpreted as just ClpString or just VarString. E.g.:
NOT msg<ClpString,VarString>: "varstring" -> msg<ClpString> exists OR (NOT msg<VarString>: "varstring")
NOT msg<ClpString,VarString>: "clp string" -> (NOT msg<ClpString>: "clp string") OR msg<VarString> exists.

Need to think harder about whether that second point matters though.

Thought about it a bit more -- I think it might be worth adding an option to NarrowTypes to disable the transformation into separate ClpString and VarString predicates, and use that option in the clp+ path. Since clp+ currently uses NodeType::VarString for captured variables we can produce VarString column entries that contain a space, which means that the transformation isn't even necessarily correct unless we also change column resolution to be aware of that, and even if we do make column resolution account for that we lose the perf benefit we get in clp-s from this transformation, so there's really no point in doing it for clp+.

gibber9809

Read through the JsonParser ingestion path and left some comments. Going to make sure I at least also take a look at ArchiveWriter changes before I leave. Should be able to catch any format changes I'm worried about that way, which is the only kind of change I'm actually worried about for release-blocking.

gibber9809 · 2026-07-03T20:15:36Z

                } else {
                    node_id = m_archive_writer
                                      ->add_node(node_id_stack.top(), NodeType::VarString, cur_key);
+                    m_current_parsed_message.add_unordered_value(value);


Applies to all of these blocks that check if a string contains a space before deciding how to encode it, but did we ever do the experiment where we encode all strings in clp+ using the log-mechanic path?

Only reason I'm bringing this up is that if we decide to do this later on (e.g., because handling everything with log-mechanic might make it simpler to do things like PII removal across all string fields), then we'll probably need to add a lot of special cases in the code for archive versions before and after that change.

gibber9809 · 2026-07-03T20:22:41Z

+        return std::nullopt;
+    }
+
+    if (m_retain_float_format) {


I think it might make sense to force users to retain float formats for floats inside of strings, since we're supposed to compress string fields losslessly.

gibber9809 · 2026-07-03T20:51:00Z

+        auto parent_node_id{get_parent_schema_node(match.value(), log_msg_node_id)};
+
+        auto const rule_name{match->ffi_pointers.rule_name.as_cpp_view()};
+        auto const lexeme{match->ffi_pointers.lexeme.as_cpp_view()};
+
+        SchemaNode::id_t node_id{0};
+        if (0 < match->encoding_idx) {
+            auto const matched_encodings{m_log_surgeon_parser->get_encoding(match->encoding_idx)};
+            for (auto const& encoding : matched_encodings) {
+                if (clpp::cFloatEncodingName == encoding) {
+                    if (auto const float_node_id{
+                                try_add_float_value(lexeme, parent_node_id, rule_name)
+                        };
+                        float_node_id.has_value())
+                    {
+                        node_id = float_node_id.value();
+                        break;
+                    }
+                } else if (clpp::cIntEncodingName == encoding) {
+                    if (encoded_variable_t var{0}; clp::EncodedVariableInterpreter::
+                                convert_string_to_representable_integer_var(lexeme, var))
+                    {
+                        node_id = m_archive_writer
+                                          ->add_node(parent_node_id, NodeType::Integer, rule_name);
+                        m_current_parsed_message.add_unordered_value(var);
+                        break;
+                    }
+                }
+            }
+        }
+        if (0 == node_id) {
+            node_id = m_archive_writer->add_node(parent_node_id, NodeType::VarString, rule_name);
+            m_current_parsed_message.add_unordered_value(lexeme);
+        }
+        m_current_schema.insert_unordered(node_id);


I think I kind of follow how the schema + MPT are being handled here, but I don't 100% get it.

MPT side I'm pretty sure I understand -- we create nodes in the MPT for parent rules with a special node type, which is more or less what I would expect.

Schema side it looks like we're adding nodes to the schema for each parent rule? If I'm interpreting the code correctly, it looks like for a rule a.b.c the schema tree becomes a -> b -> c and the schema becomes node id<a>, node id<b>, node id<c>? And if c has a sibling d (as in they're both subrules of the subrule a.b, and appear as part of the same parent match as opposed to two different instances of a.b being matched) the schema becomes node id<a>, node id<b>, node id<c>, node id<a>, node id<b>, node id<d>? Not sure if I'm completely following the code though.

The main thing I don't understand is how we figure out that c and d are siblings both for decompression, and for supporting search on parent rules to make sure we turn it into leaf queries on c and d that are part of the same parent match. If we don't currently have a good way of figuring this out, I think we'll need one once we implement a version of parent search that ensures that the resulting leaf queries target leafs from the same parent match.

I think what I would have expected is for us to use start_unordered_object/end_unordered_object for each parent rule to create schemas like:
<ParentRule, 3>, <ParentRule, 2>, node_id<c>, node_id<d>
or
<ParentRule, 5>, node_id<a>, <ParentRule, 3>, node_id<b>, node_id<c>, node_id<d>
if including the parent node IDs in the schema helps make parent search simpler.

Either way though, it lets the schema record whether some arbitrary group of leaf nodes is part of the same parent match.

You're analysis was right that the code couldn't handle it.

The recent update should resolve this by switching parent rules to use start/end_unordered_object.

It was a good point that although the syntax can't expose this limitation yet, we will benefit by having the format ready so that "old" archives are usable when we support it.

gibber9809

Did another round of review mostly looking through the ArchiveWriter. I think I've covered everything I'm worried about on the format side now.

gibber9809 · 2026-07-06T20:02:28Z

+    /**
+     * @return The root node ID of the LogType sub-tree.
+     * @return -1 if the sub-tree does not exist.
+     */
+    auto get_logtype_node() -> SchemaNode::id_t {
+        return m_schema_tree.get_subtree_node_id(constants::cDefaultNamespace, NodeType::LogType);
+    }


Nit: not used anymore? IIRC this is an artifact of an older version of the design.

You're right. It should be removed now.
There might be a few of these sitting around. I still need to do a thorough pass of all changes in the branch.

gibber9809 · 2026-07-06T20:19:43Z

+        );
+    }
+
+    if (false == m_parsing_spec_str.empty()) {


Potential nit, is an empty string technically a legal schema? If so, might make sense to use optional for the parsing spec as well.

gibber9809 · 2026-07-06T20:20:46Z

+    } else {
+        log_dict_size = m_log_dict->get_data_size();
+    }
+    return log_dict_size + m_var_dict->get_data_size() + m_array_dict->get_data_size()


Is that log shape stats array a meaningful size? May want to include that here as well if so.

gibber9809 · 2026-07-06T20:31:13Z

+        if (compressed_size.has_error()) {
+            throw OperationFailed(ErrorCodeFailure, __FILENAME__, __LINE__);
+        }
+        files.emplace_back(


This one is pretty important -- the order that different components are packed into the single-file archive is determined by the order of entries of this files array. That means that by doing emplace_back for this and the other new sections we're putting them at the end of the archive after the ERTs. For local search that's fine, but for search from S3 that would e.g. force us to buffer the entire archive in memory so that we can read the parsing spec before we can go back and actually do search.

Generally, we want this array to be sorted in the exact order that we would access these sections during search, so that we can do early termination when streaming the archive from S3. I think we'd probably want to move the parsing spec either before or around the schema tree/schema map, and move the parent rule shapes/log shape stats before or around the dictionaries? You have a better understanding of the requirements for clpp search, so I'll leave the exact order up to you.

gibber9809 · 2026-07-06T20:36:20Z

+}
+
+auto ArchiveReaderAdaptor::is_experimental_archive() const -> bool {
+    return has_section(constants::cArchiveParentRuleShapesFile);


This works fine for distinguishing clp-s and clpp archives, but do you think it would also make sense to use some of the unused header bytes to indicate as much, or record it in one of the metadata packets?

gibber9809 · 2026-07-06T20:44:28Z

+    log_shape.append(log_msg.substr(log_msg_pos));
+
+    auto [log_shape_id, new_log_shape]{
+            YSTDLIB_ERROR_HANDLING_TRYX(m_archive_writer->update_log_shape_dict(log_shape))


I could be missing something, but do we have to do escaping for these log shapes? I would have assumed so, but I don't see it happening in this code. If we do, it might be nice to have a class or something to represent an escaped log shape, since it's a bit hard to audit whether escaping rules are being respected when we're building up the log shape as an std::string directly.

You're right I just hadn't gotten to it till recently and I hadn't pushed yet. This should be addressed now.

… containing their child rules; fix decompression

junhaoliao and others added 30 commits January 17, 2026 10:06

feat(clp-package): Add configurable services restart policy for the P…

59bd992

…ackage Docker Compose project (resolves y-scope#1747). (y-scope#1773) Co-authored-by: sitaowang1998 <sitaowang1998@outlook.com>

fix(clp-package): Add missing archive output and storage engine setti…

4bdfc35

…ngs to server configuration (fixes y-scope#1799). (y-scope#1803) Co-authored-by: davemarco <83603688+davemarco@users.noreply.github.com>

fix(docs): Upgrade sphinxcontrib-mermaid to v2.0.0; Adjust configur…

3e48f1e

…ation for new schema (fixes y-scope#1866). (y-scope#1867)

fix(webui): Fix dataset handling in compression job creation to use d…

b55f140

…efault when value is empty (fixes y-scope#1854). (y-scope#1855)

feat(core): Optimize subquery generation by narrowing wildcard token …

ef64a1c

…type inference (fixes y-scope#1864). (y-scope#1865) Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>

feat(helm): Add multi-node deployment support with configurable sched…

a02bb70

…uling. (y-scope#1829) Co-authored-by: Devin Gibson <gibber9809@users.noreply.github.com> Co-authored-by: Kirk Rodrigues <2454684+kirkrodrigues@users.noreply.github.com>

fix(clp-package): Ensure --count and --count-by-time arguments ar…

b3b9914

…e mutually exclusive in sbin search scripts (fixes y-scope#1859). (y-scope#1871)

Update log surgeon to latest.

5fee121

Merge remote-tracking branch 'upstream/main' into capture-support

0fe2ac4

Forgot to lint.

9a1ccbe

Revert accidental dev-utils change.

1b6d0c6

Merge remote-tracking branch 'upstream/main' into capture-support

faa36f1

Add LogTypeDictionaryEntry::add_static_text and do minor linting/refa…

3bac58f

…ctoring.

Merge remote-tracking branch 'upstream/main' into capture-support

e621e83

Add fallback to storing a token as static text if there is no capture…

3b68d9a

… match.

Missing var_ids.

936d9b0

Add fallback to static text if log surgeon returns an empty capture l…

85f6b48

…ist (hopfully not possible).

Merge remote-tracking branch 'upstream/main' into capture-support

1016d84

Merge branch 'capture-support' into clpsls-prototype

c07f4a2

Fix unit test compilation error using clps archive reader options.

eca75a1

Switch to new log surgeon capture API.

330a13e

Rename variables to drop unnecessary/confusing _view suffix.

4a5a00a

doh

89e4b32

Address coderabit nit by switching token.to_string to token.to_string…

1e09594

…_view.

Merge remote-tracking branch 'upstream/main' into capture-support

9ce8524

Remove redundant else branch added by mistake.

3ade8e4

Merge branch 'capture-support' into clpsls-prototype

07bc920

Small style changes.

abbae8a

Merge remote-tracking branch 'upstream/main' into clpsls-prototype

ccdd975

Remove obsolete method.

8bb618d

davidlion and others added 21 commits June 16, 2026 21:57

Fix log-converter regex issue with possible empty branches; still fai…

534fb00

…ls due to parsing the next event's timestamp.

bump log-surgeon

7e6fbd6

Merge branch 'use-ls-rust' into clpp

5d18c74

missed changes from log-surgeon bump in merge; renaming

87102e1

lint kql.hpp

7f308f5

bump log surgeon

b2f4eb7

boop

406c0c9

Merge remote-tracking branch 'upstream/main' into clpp

25e8ae9

Update stats; misc tweaks

de12358

Bump log surgeon; use parent pointer; update spec tempalte.

6430124

Add shape filtering; swap to function syntax; misc tweaks.

d4b103f

Prints to remove later.

1353eae

Merge remote-tracking branch 'upstream/main' into clpp

ecc8c0e

Bump log-surgeon; update heuristic spec.

f6c1c8e

bump log surgeon for parser reset call; log converter passing integra…

7a0313a

…tion tests

ls jit on by default

2f21524

Merge remote-tracking branch 'upstream/main' into clpp

8bb955c

Changed create_output_handler refactor.

drop from_chars for macos

d970d80

remove stats logging

91734fb

toolchains:rust tweak

a58cc97

fix docs reference

8297c28

gibber9809 reviewed Jun 29, 2026

View reviewed changes

gibber9809 reviewed Jul 3, 2026

View reviewed changes

gibber9809 reviewed Jul 6, 2026

View reviewed changes

davidlion added 6 commits July 7, 2026 02:06

properly escape log shape and update parent shapes accordingly.

813a260

archive recognition fix

5bf2b03

Fix for adjacent placeholders.

445b94f

remove deadcode

7df513e

fix rule scoping issue so that parent rules are now unordered objects…

0745b46

… containing their child rules; fix decompression

Merge remote-tracking branch 'upstream/main' into clpp

c1a615a

Uh oh!

Conversation

davidlion commented May 12, 2026

Description

Checklist

Validation performed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibber9809 Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibber9809 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibber9809 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gibber9809 Jul 5, 2026 •

edited

Loading