perf: Improve expr to subfield filters #15361

MBkkt · 2025-11-01T07:32:01Z

What is done?

Different handling of "and", "or", "not"
Different makeOr
Use real notEqual in more cases

Why is it good?

See Axiom tpch q19

Before

Executable Velox plan:

Fragment 0:  numWorkers=0:
-- Aggregation[5][SINGLE revenue := sum("dt1.__p121")] -> revenue:DOUBLE
  -- Project[4][expressions: (dt1.__p121:DOUBLE, multiply("l_extendedprice",minus(1,"l_discount")))] -> "dt1.__p121":DOUBLE
    -- Filter[0][expression: or(and(between(cast("p_size" as BIGINT),1,15),and(lte("l_quantity",30),and(gte("l_quantity",20),and(eq("p_brand",Brand#34),in("p_container",{LG CASE, LG BOX, LG PACK, LG PKG}))))),or(and(between(cast("p_size" as BIGINT),1,5),and(lte("l_quantity",11),and(gte("l_quantity",1),and(eq("p_brand",Brand#12),in("p_container",{SM CASE, SM BOX, SM PACK, SM PKG}))))),and(between(cast("p_size" as BIGINT),1,10),and(lte("l_quantity",20),and(gte("l_quantity",10),and(eq("p_brand",Brand#23),in("p_container",{MED BAG, MED BOX, MED PKG, MED PACK})))))))] -> l_quantity:DOUBLE, l_extendedprice:DOUBLE, l_discount:DOUBLE, p_brand:VARCHAR, p_size:INTEGER, p_container:VARCHAR
      -- HashJoin[3][INNER l_partkey=p_partkey] -> l_quantity:DOUBLE, l_extendedprice:DOUBLE, l_discount:DOUBLE, p_brand:VARCHAR, p_size:INTEGER, p_container:VARCHAR
        -- TableScan[1][table: lineitem, range filters: [(l_shipinstruct, Filter(BytesValues, deterministic, null not allowed)), (l_shipmode, Filter(BytesValues, deterministic, null not allowed))], remaining filter: (or(and(gte("l_quantity",20),lte("l_quantity",30)),or(and(gte("l_quantity",1),lte("l_quantity",11)),and(gte("l_quantity",10),lte("l_quantity",20))))), data columns: ROW<l_orderkey:BIGINT,l_partkey:BIGINT,l_suppkey:BIGINT,l_linenumber:INTEGER,l_quantity:DOUBLE,l_extendedprice:DOUBLE,l_discount:DOUBLE,l_tax:DOUBLE,l_returnflag:VARCHAR,l_linestatus:VARCHAR,l_shipdate:DATE,l_commitdate:DATE,l_receiptdate:DATE,l_shipinstruct:VARCHAR,l_shipmode:VARCHAR,l_comment:VARCHAR>] -> l_partkey:BIGINT, l_quantity:DOUBLE, l_extendedprice:DOUBLE, l_discount:DOUBLE
        -- TableScan[2][table: part, remaining filter: (or(and(between(cast("p_size" as BIGINT),1,15),and(eq("p_brand",Brand#34),in("p_container",{LG CASE, LG BOX, LG PACK, LG PKG}))),or(and(between(cast("p_size" as BIGINT),1,5),and(eq("p_brand",Brand#12),in("p_container",{SM CASE, SM BOX, SM PACK, SM PKG}))),and(between(cast("p_size" as BIGINT),1,10),and(eq("p_brand",Brand#23),in("p_container",{MED BAG, MED BOX, MED PKG, MED PACK})))))), data columns: ROW<p_partkey:BIGINT,p_name:VARCHAR,p_mfgr:VARCHAR,p_brand:VARCHAR,p_type:VARCHAR,p_size:INTEGER,p_container:VARCHAR,p_retailprice:DOUBLE,p_comment:VARCHAR>] -> p_partkey:BIGINT, p_brand:VARCHAR, p_size:INTEGER, p_container:VARCHAR

After

Executable Velox plan:

Fragment 0:  numWorkers=0:
-- Aggregation[5][SINGLE revenue := sum("dt1.__p121")] -> revenue:DOUBLE
  -- Project[4][expressions: (dt1.__p121:DOUBLE, multiply("l_extendedprice",minus(1,"l_discount")))] -> "dt1.__p121":DOUBLE
    -- Filter[0][expression: or(and(between(cast("p_size" as BIGINT),1,15),and(lte("l_quantity",30),and(gte("l_quantity",20),and(eq("p_brand",Brand#34),in("p_container",{LG CASE, LG BOX, LG PACK, LG PKG}))))),or(and(between(cast("p_size" as BIGINT),1,5),and(lte("l_quantity",11),and(gte("l_quantity",1),and(eq("p_brand",Brand#12),in("p_container",{SM CASE, SM BOX, SM PACK, SM PKG}))))),and(between(cast("p_size" as BIGINT),1,10),and(lte("l_quantity",20),and(gte("l_quantity",10),and(eq("p_brand",Brand#23),in("p_container",{MED BAG, MED BOX, MED PKG, MED PACK})))))))] -> l_quantity:DOUBLE, l_extendedprice:DOUBLE, l_discount:DOUBLE, p_brand:VARCHAR, p_size:INTEGER, p_container:VARCHAR
      -- HashJoin[3][INNER l_partkey=p_partkey] -> l_quantity:DOUBLE, l_extendedprice:DOUBLE, l_discount:DOUBLE, p_brand:VARCHAR, p_size:INTEGER, p_container:VARCHAR
        -- TableScan[1][table: lineitem, range filters: [(l_quantity, Filter(MultiRange, deterministic, null not allowed)), (l_shipinstruct, Filter(BytesValues, deterministic, null not allowed)), (l_shipmode, Filter(BytesValues, deterministic, null not allowed))], data columns: ROW<l_orderkey:BIGINT,l_partkey:BIGINT,l_suppkey:BIGINT,l_linenumber:INTEGER,l_quantity:DOUBLE,l_extendedprice:DOUBLE,l_discount:DOUBLE,l_tax:DOUBLE,l_returnflag:VARCHAR,l_linestatus:VARCHAR,l_shipdate:DATE,l_commitdate:DATE,l_receiptdate:DATE,l_shipinstruct:VARCHAR,l_shipmode:VARCHAR,l_comment:VARCHAR>] -> l_partkey:BIGINT, l_quantity:DOUBLE, l_extendedprice:DOUBLE, l_discount:DOUBLE
        -- TableScan[2][table: part, remaining filter: (or(and(between(cast("p_size" as BIGINT),1,15),and(eq("p_brand",Brand#34),in("p_container",{LG CASE, LG BOX, LG PACK, LG PKG}))),or(and(between(cast("p_size" as BIGINT),1,5),and(eq("p_brand",Brand#12),in("p_container",{SM CASE, SM BOX, SM PACK, SM PKG}))),and(between(cast("p_size" as BIGINT),1,10),and(eq("p_brand",Brand#23),in("p_container",{MED BAG, MED BOX, MED PKG, MED PACK})))))), data columns: ROW<p_partkey:BIGINT,p_name:VARCHAR,p_mfgr:VARCHAR,p_brand:VARCHAR,p_type:VARCHAR,p_size:INTEGER,p_container:VARCHAR,p_retailprice:DOUBLE,p_comment:VARCHAR>] -> p_partkey:BIGINT, p_brand:VARCHAR, p_size:INTEGER, p_container:VARCHAR

netlify · 2025-11-01T07:32:07Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`af87b76`
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/690cc2e550d1220008210244

mbasmanova · 2025-11-03T06:52:38Z

velox/expression/ExprToSubfieldFilter.h

    const core::TypedExprPtr& expr,
-    core::ExpressionEvaluator*);
+    core::ExpressionEvaluator* evaluator,
+    bool negated = false);


It looks like this PR may combine multiple changes. Some might be optimizations that do not change behavior, others are API changes. Would it be possible to split these into separate PRs and add tests for newly introduced functionality?

This doesn't change API

This just add recursive case, it can be implemented as separate recursive function in anonymous namespace, but why? So I think api wasn't changed

This function is exposed in the header file, hence, I assume it is public API. It used to have 2 parameters, now it has 3. This seems like a change. If we do not expect users to specify the 3rd argument, then we should remove it and add a helper function to .cpp file.

I see this API wasn't documented before. Would you help document it?

This function is exposed in the header file, hence, I assume it is public API. It used to have 2 parameters, now it has 3. This seems like a change.

It's has default value, so it's not changed, it's extended.

I see this API wasn't documented before. Would you help document it?

ok

Yuhta · 2025-11-03T16:45:27Z

velox/expression/ExprToSubfieldFilter.cpp

+      return std::make_unique<common::BigintMultiRange>(
+          std::move(newRanges), false);
+    } catch (...) {
+      // Found overlapping ranges, fall back to MultiRange.


Log some WARNING here

It's not warning? We just didn't write code that merge overlapping ranges, it doesn't mean something wrong with user request

velox/expression/ExprToSubfieldFilter.cpp

Yuhta · 2025-11-03T16:52:12Z

velox/expression/ExprToSubfieldFilter.cpp

+  bool conjunction = call->name() == "and";
+  if (conjunction || call->name() == "or") {
+    if (call->inputs().empty()) {
+      return {};


Always true/false

It's wrong, I made comment why.

Yuhta · 2025-11-03T16:54:05Z

velox/expression/ExprToSubfieldFilter.cpp

-    return bigintOr(
-        asUniquePtr<common::BigintRange>(std::move(a)),
-        asUniquePtr<common::BigintRange>(std::move(b)));
+  VELOX_DCHECK_NOT_NULL(a);


The change on makeOrFilter probably should be a separate PR

I don't have a lot of time for this PR.
I made this in my free time, just because it was easy to fix and looks overall better.

What can we do with this?

@MBkkt Maybe you can try to find someone else who'd be interested in picking up this work. Otherwise, may have to abandon.

@mbasmanova I created the issue #15426

If you think better to have code with all these flaws there's nothing else what I can do without allocating additional time on this.

Yuhta · 2025-11-03T16:57:07Z

velox/expression/ExprToSubfieldFilter.cpp

-
-  std::unique_ptr<common::Filter> lessThanFilter =
-      makeLessThanFilter(valueExpr, evaluator);
+  switch (value->typeKind()) {


The change on makeNotEqualFilter is another PR

I don't have a lot of time for this PR.
I made this in my free time, just because it was easy to fix and looks overall better.

What can we do with this?

meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 1, 2025

MBkkt force-pushed the mbkkt/improve-subfield-filters branch 2 times, most recently from ab6aac8 to d8824ad Compare November 1, 2025 13:17

MBkkt requested review from Yuhta and mbasmanova November 1, 2025 13:21

MBkkt mentioned this pull request Nov 1, 2025

Inspect TPC-H plans and flag bad ones facebookincubator/axiom#582

Open

MBkkt force-pushed the mbkkt/improve-subfield-filters branch from d8824ad to 1daf10f Compare November 1, 2025 14:05

mbasmanova reviewed Nov 3, 2025

View reviewed changes

MBkkt force-pushed the mbkkt/improve-subfield-filters branch from 1daf10f to 1645bdc Compare November 3, 2025 10:55

Yuhta changed the title ~~perf: Improve subfield filters~~ perf: Improve expr to subfield filters Nov 3, 2025

Yuhta reviewed Nov 3, 2025

View reviewed changes

MBkkt force-pushed the mbkkt/improve-subfield-filters branch 3 times, most recently from f341c39 to 8703b32 Compare November 6, 2025 13:26

perf: Improve subfield filters

af87b76

MBkkt force-pushed the mbkkt/improve-subfield-filters branch from a1db486 to af87b76 Compare November 6, 2025 15:46

MBkkt mentioned this pull request Nov 6, 2025

Expression to subfield filter code doesn't work in many cases #15426

Open

10 tasks

perf: Improve expr to subfield filters #15361

Are you sure you want to change the base?

perf: Improve expr to subfield filters #15361

Conversation

MBkkt commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

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!

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MBkkt commented Nov 1, 2025 •

edited

Loading

netlify bot commented Nov 1, 2025 •

edited

Loading