Bug fixes for query builder filtering with ISIN filters over sets with duplicate str representations by poodlewars · Pull Request #3201 · man-group/ArcticDB

poodlewars · 2026-06-25T12:15:38Z

Bug 1

Fix for OR filters with isin with value sets with clashing str representations:

q = q[(q["a"].isin(vals1)) | (q["a"].isin(vals2))]

str(np.ndarray) truncates like [0, 1, 2, ... 99997, 99998, 99999] so it is easy for ValueSet str representations to clash. We have special handling for this to suffix -vX in the ValueSet leaf node, but there was no such handling for the ExpressionNode,

ISIN([VALUE_SET_1])

This meant that filters,

q["a"].isin(V1) | q["a"].isin(V2)

would incorrectly create a single node key, ISIN [1, 2 ... 999] and one of the two filters would effectively be dropped at evaluation time, since we used these strings to describe the edges in the AST.

Bug 2

Drop release notes for this part - not user facing as column stats is feature flagged. Fixes expressions like this with column stats in the case where the two value sets have the same str representation:

    q = QueryBuilder()
    q = q[q["col_1"].isin(value_set_one)]
    q = q[q["col_1"].isnotin(value_set_two)]

Column stats AND-s filters together in to one in and_filter_expression_contexts., which merged the trees in two ExpressionContext objects together in to a graph, sharing leaf nodes where they matched on names.

These names in the original filter AST come from this in processing.py:

def to_string(leaf):
    if isinstance(leaf, (np.ndarray, list)):
        # Truncate value set keys to first 100 characters
        key = str(leaf)[:100]
    elif isinstance(leaf, _RegexGeneric):
        key = "Regex({})".format(leaf.text())
    else:
        if isinstance(leaf, str):
            key = "Str({})".format(leaf)
        elif isinstance(leaf, bool):
            key = "Bool({})".format(leaf)
        else:
            key = "Num({})".format(leaf)
    return key

so different ValueSets can clash, leading to an incorrect merged tree . Note str(np.ndarray) also truncates like [0, 1, 2, ... 99997, 99998, 99999], so the "first 100 chars" is not the only problem.

Fix

We now form the query AST as a graph in the frontend, rather than relying on string references to encode edges. This gets rid of the need to make the two fixes below.

We keep the concept of memoizing results, ie making sure we only evaluate subtrees once in cases like:

  # only want to calculate the isin once
  big = list(range(100_000))
  q = q[q["id"].isin(big) & (q["t"] > 0) | q["id"].isin(big)]

  # only want to calculate the spread once
  spread = q["bid"] - q["ask"]
  q = q[(spread > 0) & (spread < limit)]

We do this by looking up expressions that we are going to evaluate in a map of string label name to expression node and result. To guard against conflicts, we do a deep comparison against the node that created the result.

claude · 2026-06-25T12:18:25Z

ArcticDB Code Review Summary

Reviewed the latest delta (58f87b9..d7e8a46, single commit "Implement PR comments"). Note the diff range was rebased onto a newer master, so it also carries ~40 unrelated master files (incompletes, python_to_tensor_frame, frame_utils, arrow, etc.); those are identical to master and are not part of this PR. The genuine authored changes are all responses to prior review comments and are correct:

Operation members reordered to operation_type_, left_, right_, condition_ with a defaulted-arg constructor; all three ExpressionNode ctors updated to match — verified argument mapping is correct.
Removed noisy per-evaluation memoization debug logs; kept a single debug log only when try_emplace fails on a genuine label collision.
to_string moved to a @staticmethod on ExpressionNode (Python).
and_filter_expression_contexts now propagates dynamic_schema_ from the input contexts and asserts they agree, with new test_query_planner.cpp covering true/false/single/inconsistent cases.
Added lifetime comment on computed_data_ explaining raw-pointer validity.
Tests migrated to create_dense_segment, contains over count, ExpressionChild right = variant_match(...), and the reused-derived-expression test now produces a non-empty result.
ast_test_helpers.hpp and test_query_planner.cpp added to CMake test sources.

Resolved in this delta (carried-over doc items)

✅ docs/claude/cpp/PROCESSING.md — the stale ExpressionContext::merge_from() / ConstantMap::contains() documentation is replaced with the direct graph representation (root_) and and_filter_expression_contexts().
✅ docs/claude/cpp/PROCESSING.md — computed_data_ doc expanded to describe the current scheme (keyed on label_, one slot per label, deep-compared on a hit).

Resolved (earlier)

✅ Performance — subexpression memoization is in place.
✅ Correctness — Bug 1/Bug 2 (ISIN value-set label clashes) fixed by building the AST as a graph in the frontend.

No new issues in this delta.

poodlewars · 2026-06-25T12:56:40Z

+    assert table_data_reads == expected_reads, f"Expected {expected_reads} TABLE_DATA read(s), got {table_data_reads}"
+
+
+def test_column_stats_isin_and_isnotin_colliding_value_sets_nonreg(


This is the only test that was failing before the change. I think the ones above are worth adding anyway.

alexowens90 · 2026-07-01T10:24:46Z

 #include <arcticdb/processing/expression_node.hpp>
-#include <arcticdb/pipeline/value.hpp>
-#include <arcticdb/pipeline/value_set.hpp>
+#include <arcticdb/util/preconditions.hpp>


alexowens90 · 2026-07-01T10:31:16Z

+        std::shared_ptr<ExpressionNode> condition_;
+        std::shared_ptr<ExpressionNode> left_;
+        std::shared_ptr<ExpressionNode> right_;
+    };


nit: I think it would look slightly cleaner if the member ordering was operation_type_, left_, right_, condition_, with a ctor with defaulted nullptr values for right_ and condition_

alexowens90 · 2026-07-01T10:33:55Z

+                    }
+                    log::version().debug("Memoization label collision for {}", label_);
+                }
+                log::version().debug("Memoization miss, computing {}", label_);


This will be very noisy, I'd either remove or compile out of debug builds

alexowens90 · 2026-07-01T10:35:46Z

+                }();
+                // On a label collision the slot is already held by a structurally-different node; try_emplace is a
+                // no-op there, so that node keeps the slot and this result simply isn't memoized.
+                seg.computed_data_.try_emplace(label_, this, result);


I think it is worth logging if this emplace fails, as you could conceivably see unexpected bimodal performance for similar looking queries in this case

alexowens90 · 2026-07-01T10:42:49Z

+        return false;
+    }
+    if (a.base_type() != b.base_type()) {
+        // This might cause some false negatives but the simplicity is worth it


alexowens90 · 2026-07-01T10:44:03Z

+                leaf_a,
+                [&leaf_b](const ColumnName& a_col) { return a_col.value == std::get<ColumnName>(leaf_b).value; },
+                [&leaf_b](const std::shared_ptr<Value>& a_val) {
+                    return *a_val == *std::get<std::shared_ptr<Value>>(leaf_b);


SImilarly to the false negatives comment above, this wouldn't consider 2 semantically equal but representatively different values as equal (e.g. np.uint8(0) and np.uint16(0), but agree it isn't worth worrying about

alexowens90 · 2026-07-01T10:44:39Z

+        );
+    }
+    const auto& oa = std::get<ExpressionNode::Operation>(a.kind_);
+    const auto& ob = std::get<ExpressionNode::Operation>(b.kind_);


nit: operation_a and operation_b

alexowens90 · 2026-07-01T10:52:46Z

+    );
+}
+
+std::string label_for(const ExpressionNode::Operation& op) {


Guessing this works because by construction child nodes are always created before parent nodes?

Yeah, everything is built bottom up.

alexowens90 · 2026-07-01T10:59:49Z

-                util::raise_rte("ProcessingUnit::get called with monostate VariantNode");
-            }
-    );
+    if (expression_context_ && !expression_context_->dynamic_schema_) {


Is this the only place the dynamic_schema_ member of ExpressionContext is used? If so, clauses know whether dynamic schema is enabled or not via the set_processing_config method, so we could scrap ExpressionContext entirely

alexowens90 · 2026-07-01T11:01:25Z

+     * Memoization of results after applying an ExpressionNode to this processing unit. Keyed on the node's label_ with
+     * a deep comparison on hits. One slot per label, colliding writes are dropped.
+     */
+    std::unordered_map<std::string, std::pair<const ExpressionNode*, VariantData>> computed_data_;


Maybe worth a comment that the shared pointer owning the ExpressionNode is guaranteed to outlive this map, and so the raw pointer is safe

alexowens90 · 2026-07-01T11:02:45Z

+    for (const auto& expression_context : expression_contexts) {
        util::check(
-                std::holds_alternative<ExpressionName>(expression_context->root_node_name_),
+                expression_context->root_ && expression_context->root_->is_operation(),


I think the second condition is wrong, you can filter directly on a bool column for example

FilterClause already has this assertion,

explicit FilterClause( std::unordered_set<std::string> input_columns, ExpressionContext expression_context, std::optional<PipelineOptimisation> optimisation ) : expression_context_(std::make_shared<ExpressionContext>(std::move(expression_context))), optimisation_(optimisation.value_or(PipelineOptimisation::SPEED)) { user_input::check<ErrorCode::E_INVALID_USER_ARGUMENT>( expression_context_->root_ && expression_context_->root_->is_operation(), "FilterClause AST would produce a column, not a bitset" ); clause_info_.input_columns_ = std::move(input_columns); }

It works because the bare bool filter gets wrapped in an IDENTITY operation in the Python layer.

alexowens90 · 2026-07-01T11:04:15Z

    }

-    res.root_node_name_ = *overall_root_name;
+    ExpressionContext res;


If we keep the dynamic_schema_ field, then presumably should be inherited from the input expression contexts? Or are they not set yet?

alexowens90 · 2026-07-01T11:09:01Z

Think this is just used by the (Python) ExpressionNode class now, so could be a static method

alexowens90 · 2026-07-01T11:15:25Z

+
+    pandas_spread = df["bid"] - df["ask"]
+    expected = df[(pandas_spread > 0) & (pandas_spread < limit)].reset_index(drop=True)
+    received = lib.read(sym, query_builder=q).data


This will always be empty as the spread is always -5, might be more interesting to test a case with a non-empty result

alexowens90 · 2026-07-01T11:23:13Z

nit: should be in unit_test_srcs

alexowens90 · 2026-07-01T11:29:34Z

+    const std::string add_label = R"((Column["thing1"] ADD Column["thing2"]))";
+    ASSERT_EQ(add1->label_, add_label);
+    ASSERT_EQ(add2->label_, add_label);
+    ASSERT_EQ(proc.computed_data_.count(add_label), 1);


nit: contains over count == 1 everywhere

alexowens90 · 2026-07-01T11:31:57Z

-        expression_context->root_node_name_ = ExpressionName(root_node_name);
-        std::shared_ptr<ExpressionNode> expression_node;
+        ExpressionChild right;
        util::variant_match(


Suggested change

util::variant_match(

ExpressionChild right = util::variant_match(

vasil-pashov · 2026-07-02T13:12:37Z

+    auto wrapper =
+            SinkWrapper(symbol, {scalar_field(DataType::UINT64, "thing1"), scalar_field(DataType::UINT64, "thing2")});
+
+    for (auto j = 0; j < 20; ++j) {
+        wrapper.aggregator_.start_row(timestamp(j))([&](auto&& rb) {
+            rb.set_scalar(1, j);
+            rb.set_scalar(2, j + 1);
+        });
+    }


What do you think about using create_dense_segment from segment_generation_utils.hpp instead? I find it a bit more readable. It'll look something like

SegmentInMemory seg = create_dense_segment( {scalar_field(DataType::UINT64, "thing1"), scalar_field(DataType::UINT64, "thing2")}, std::ranges::views::iota<uint64_t, uitnt64_t>(0, 20), std::ranges::views::iota<uint64_t, uitnt64_t>(1, 21) );

there's also slice_data_into_segments if you want to simulate how arcticdb slices segments (including getting the index as first column in every slice)

…h clashing names Column stats AND-s filters together. The names in the filter AST come from this in `processing.py`: ``` def to_string(leaf): if isinstance(leaf, (np.ndarray, list)): # Truncate value set keys to first 100 characters key = str(leaf)[:100] elif isinstance(leaf, _RegexGeneric): key = "Regex({})".format(leaf.text()) else: if isinstance(leaf, str): key = "Str({})".format(leaf) elif isinstance(leaf, bool): key = "Bool({})".format(leaf) else: key = "Num({})".format(leaf) return key ``` so different ValueSets can clash, leading to an incorrect merged tree. Note str(np.ndarray) also truncates like [0, 1, 2, ... 99997, 99998, 99999], so the "first 100 chars" is not the only problem.

…lue sets with clashing str representations. str(np.ndarray) truncates like [0, 1, 2, ... 99997, 99998, 99999] so it is easy for ValueSet str representations to clash. We have special handling for this to suffix -vX in the ValueSet leaf node, but there was no such handling for the ExpressionNode, ``` ISIN([VALUE_SET_1]) ``` This meant that filters, ``` q["a"].isin(V1) | q["a"].isin(V2) ``` would incorrectly create a single node key, `ISIN [1, 2 ... 999]` and one of the two filters would effectively be dropped at evaluation time, since we use these strings to describe the edges in the AST. This change creates separate node keys: ``` `ISIN [1, 2 ... 999]-v0` `ISIN [1, 2 ... 999]-v1` ``` Commit b38c802 fixed a similar problem across different ExpressionContexts - this fix is within a single ExpressionContext.

… string references to encode edges. This may worsen performance on some queries as we are no longer memoizing: ``` isin repeated twice now: big = list(range(100_000)) q = q[q["id"].isin(big) & (q["t"] > 0) | q["id"].isin(big)] derived expressions: spread = q["bid"] - q["ask"] q = q[(spread > 0) & (spread < limit)] ``` It is difficult to see this having a huge impact in practice.

…ExpressionNode AST

poodlewars added patch Small change, should increase patch version no-release-notes This PR shouldn't be added to release notes. labels Jun 25, 2026

poodlewars commented Jun 25, 2026

View reviewed changes

poodlewars marked this pull request as draft June 25, 2026 14:24

poodlewars added the bug Something isn't working label Jun 26, 2026

poodlewars changed the title ~~Fix for column stats: merging expression contexts with value sets with clashing names~~ Bug fixes for query builder filtering with ISIN filters over values with duplicate str representations Jun 26, 2026

poodlewars removed the no-release-notes This PR shouldn't be added to release notes. label Jun 26, 2026

poodlewars marked this pull request as ready for review June 26, 2026 08:03

poodlewars changed the title ~~Bug fixes for query builder filtering with ISIN filters over values with duplicate str representations~~ Bug fixes for query builder filtering with ISIN filters over sets with duplicate str representations Jun 26, 2026

poodlewars marked this pull request as draft June 30, 2026 10:16

poodlewars marked this pull request as ready for review June 30, 2026 14:01

poodlewars force-pushed the aseaton/column-stats/11643672444/and-clauses branch 2 times, most recently from 0cd5211 to 7993d2a Compare June 30, 2026 16:07

alexowens90 reviewed Jul 1, 2026

View reviewed changes

Comment thread cpp/arcticdb/processing/expression_node.cpp

alexowens90 reviewed Jul 1, 2026

View reviewed changes

vasil-pashov approved these changes Jul 3, 2026

View reviewed changes

Alex Seaton added 6 commits July 3, 2026 16:49

Re-introduce the memoization of results for repeated subtrees in the …

4ee320d

…ExpressionNode AST

Add a test for isin with the same values over two different columns

9fb2c3a

Implement PR comments

d7e8a46

poodlewars force-pushed the aseaton/column-stats/11643672444/and-clauses branch from 58f87b9 to d7e8a46 Compare July 3, 2026 15:49

poodlewars mentioned this pull request Jul 3, 2026

Merge filter clauses in to one expression during query builder evaluation #3204

Open

		assert table_data_reads == expected_reads, f"Expected {expected_reads} TABLE_DATA read(s), got {table_data_reads}"


		def test_column_stats_isin_and_isnotin_colliding_value_sets_nonreg(

	util::variant_match(
	ExpressionChild right = util::variant_match(

Uh oh!

Conversation

poodlewars commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug 1

Bug 2

Fix

Uh oh!

claude Bot commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ArcticDB Code Review Summary

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!

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

poodlewars commented Jun 25, 2026 •

edited

Loading

claude Bot commented Jun 25, 2026 •

edited

Loading