Speed up COALESCE significantly #120139

nik9000 · 2025-01-14T18:01:14Z

                      before              after
     (operation)   Score   Error       Score   Error  Units
 coalesce_2_noop  75.949 ± 3.961  ->   0.010 ±  0.001 ns/op  99.9%
coalesce_2_eager  99.299 ± 6.959  ->   4.292 ±  0.227 ns/op  95.7%
 coalesce_2_lazy 113.118 ± 5.747  ->  26.746 ±  0.954 ns/op  76.4%

We tend to advise folks that "COALESCE is faster than CASE", but, as of
8.16.0/#112295 that wasn't the true. I was working with someone a few
days ago to port a scripted_metric aggregation to ESQL and we saw
COALESCE taking ~60% of the time. That won't do.

The trouble is that CASE and COALESCE have to be lazy, meaning that
operations like:

COALESCE(a, 1 / b)

should never emit a warning if a is not null, even if b is 0. In
8.16/#112295 CASE grew an optimization where it could operate non-lazily
if it was flagged as "safe". This brings a similar optimization to
COALESCE, see it above as "case_2_eager", a 95.7% improvement.

It also brings and arguably more important optimization - entire-block
execution for COALESCE. The schort version is that, if the first
parameter of COALESCE returns no nulls we can return it without doing
anything lazily. There are a few more cases, but the upshot is that
COALESCE is pretyt much free in cases where long strings of results
are null or not null. That's the coalesce_2_noop line.

Finally, when there mixed null and non-null values we were using a
single builder with some fairly inefficient paths. This specializes them
per type and skips some slow null-checking where possible. That's the
coalesce_2_lazy result, a more modest 76.4%.

NOTE: These %s of improvements on COALESCE itself, or COALESCE with some load-overhead operators like +. If COALESCE isn't taking a ton time in your query don't get particularly excited about this. It's fun though.

Closes #119953

``` before after (operation) Score Error Score Error Units coalesce_2_noop 75.949 ± 3.961 -> 0.010 ± 0.001 ns/op 99.9% coalesce_2_eager 99.299 ± 6.959 -> 4.292 ± 0.227 ns/op 95.7% coalesce_2_lazy 113.118 ± 5.747 -> 26.746 ± 0.954 ns/op 76.4% ``` We tend to advise folks that "COALESCE is faster than CASE", but, as of 8.16.0/elastic#112295 that wasn't the true. I was working with someone a few days ago to port a scripted_metric aggregation to ESQL and we saw COALESCE taking ~60% of the time. That won't do. The trouble is that CASE and COALESCE have to be *lazy*, meaning that operations like: ``` COALESCE(a, 1 / b) ``` should never emit a warning if `a` is not `null`, even if `b` is `0`. In 8.16/elastic#112295 CASE grew an optimization where it could operate non-lazily if it was flagged as "safe". This brings a similar optimization to COALESCE, see it above as "case_2_eager", a 95.7% improvement. It also brings and arguably more important optimization - entire-block execution for COALESCE. The schort version is that, if the first parameter of COALESCE returns no nulls we can return it without doing anything lazily. There are a few more cases, but the upshot is that COALESCE is pretyt much *free* in cases where long strings of results are `null` or not `null`. That's the `coalesce_2_noop` line. Finally, when there mixed null and non-null values we were using a single builder with some fairly inefficient paths. This specializes them per type and skips some slow null-checking where possible. That's the `coalesce_2_lazy` result, a more modest 76.4%.

elasticsearchmachine · 2025-01-14T18:01:38Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

nik9000 · 2025-01-14T18:03:47Z

benchmarks/README.md

 ```

+Note: As of January 2025 the latest release of async profiler doesn't work
+      with out JDK but the nightly is fine.


nik9000 · 2025-01-14T18:05:10Z

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/Block.java

         * {@code endExclusive} into this builder.
+         * <p>
+         *     For single position copies see {@link IntBlockBuilder#copyFrom(IntBlock, int)},
+         *     {@link LongBlockBuilder#copyFrom(LongBlock, int)}, etc.


I'll update this to explain "because it is faster".

nik9000 · 2025-01-14T18:07:05Z

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/X-BlockBuilder.java.st

+     * </p>
+     */
+    @Override
+    public $Type$BlockBuilder copyFrom($Type$Block block, int position$if(BytesRef)$, BytesRef scratch$endif$) {


I basically just yanked this out of the regular copy code and made it easily callable for other tight loops. Folks will usually have the actual type when calling it so its going to get inlined. I think this could improve a bunch of places actually.

It also helps that it makes you deal with scratch if you are doing a BytesRef - if you are copying values one by one it's important to have your own scratch and not allocate one per.

Same comment as above.

nik9000 · 2025-01-14T18:09:39Z

...gin/esql/compute/src/test/java/org/elasticsearch/compute/data/BlockBuilderCopyFromTests.java

+                case INT -> ((IntBlockBuilder) builder).copyFrom((IntBlock) block, i);
+                case LONG -> ((LongBlockBuilder) builder).copyFrom((LongBlock) block, i);
+                default -> throw new IllegalArgumentException();
+            }


I could intentionally didn't make a version of this thing in Block.Builder because I:

Want you to know if you need a scratch.

Want you to specialize on each type in the tight loops.

nik9000 · 2025-01-14T18:11:33Z

...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/nulls/Coalesce.java

+            case NULL -> EvalOperator.CONSTANT_NULL_FACTORY;
+            case UNSUPPORTED, SHORT, BYTE, DATE_PERIOD, OBJECT, DOC_DATA_TYPE, SOURCE, TIME_DURATION, FLOAT, HALF_FLOAT, TSID_DATA_TYPE,
+                SCALED_FLOAT, PARTIAL_AGG -> throw new UnsupportedOperationException("can't be coalesced");
        };


This switch will fail to compile if you make a new type and don't include it. We intend for COALESCE to work for all types. If you are building a new type then you can stub it out as you go like you'll do with a few other evaluators.

nik9000 · 2025-01-14T18:12:49Z

...c/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/nulls/CoalesceTests.java

+     * Inserts random non-null garbage <strong>around</strong> the expected data
+     * and runs
+     */
+    public void testEvaluateWithGarbage() {


This is important for catching the case where your value is null, but the rest of the block isn't null. I had an off-by-one error in the evaluators somewhere that the standard tests weren't catching and this does.

It's not clear to me if this is worth pulling to the top level class.

GalLalouche

I have a bunch of format/style comments, but approved otherwise.

GalLalouche · 2025-01-19T16:11:35Z

benchmarks/src/main/java/org/elasticsearch/benchmark/compute/operator/EvalBenchmark.java

+            case "coalesce_2_noop", "coalesce_2_eager", "coalesce_2_lazy" -> {
+                FieldAttribute f1 = longField();
+                FieldAttribute f2 = longField();
+                Expression lhs = f1;


Consider using ternary expression here.

I think that makes this one harder to read. The if statement screams "wrap in this case" to me.

GalLalouche · 2025-01-19T16:12:52Z

benchmarks/src/main/java/org/elasticsearch/benchmark/compute/operator/EvalBenchmark.java

+                ).get(driverContext);
+                String desc = operation.endsWith("lazy") ? "CaseLazyEvaluator" : "CaseEagerEvaluator";
+                if (evaluator.toString().contains(desc) == false) {
+                    throw new IllegalArgumentException("Evaluator was [" + evaluator + "] but expected one containing [" + desc + "]");


Consider using Strings.format for these.

I find that harder to read these days.

GalLalouche · 2025-01-19T16:15:51Z

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/X-Block.java.st

        Builder copyFrom($Type$Block block, int beginInclusive, int endExclusive);

+        /**
+         * Copy the values in {@code block} at {@code position}.


Does this copy values or value (since you only give it a single position). Also, consider adding a note here about the usage of scratch.

I'll add a bigger comment. It copies all of the values at this position. If it has one value, it'll copy that. If this has many values, it'll copy that. If this is null, it'll copy that.

GalLalouche · 2025-01-19T16:16:43Z

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/X-BlockBuilder.java.st

+     * </p>
+     */
+    @Override
+    public $Type$BlockBuilder copyFrom($Type$Block block, int position$if(BytesRef)$, BytesRef scratch$endif$) {


Same comment as above.

GalLalouche · 2025-01-19T16:22:28Z

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/EvalOperator.java

+
+                @Override
+                public String toString() {
+                    return "ConstantNull";


Perhaps extract this string to a constant, so it's clear it matches the one below it.

GalLalouche · 2025-01-19T16:26:51Z

...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/nulls/Coalesce.java

+                CoalesceBytesRefEvaluator.toEvaluator(toEvaluator, children());
+            case NULL -> EvalOperator.CONSTANT_NULL_FACTORY;
+            case UNSUPPORTED, SHORT, BYTE, DATE_PERIOD, OBJECT, DOC_DATA_TYPE, SOURCE, TIME_DURATION, FLOAT, HALF_FLOAT, TSID_DATA_TYPE,
+                SCALED_FLOAT, PARTIAL_AGG -> throw new UnsupportedOperationException("can't be coalesced");


Add the type to the exception message.

GalLalouche · 2025-01-19T16:29:49Z

...c/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/nulls/CoalesceTests.java

+                try (Block.Builder builder = elementType.newBlockBuilder(positions, context.blockFactory())) {
+                    for (int p = 0; p < positions; p++) {
+                        if (p == realPosition) {
+                            builder.copyFrom(onePositionPage.getBlock(b), 0, 1);


Can be extracted to builder.copyFrom(p == realPosition ? something : somethingElse, 0, 1);.
The first argument can also be extract to a local variable which will probably make the code take fewer lines.

I'll flip it some, yeah. fewer lines doesn't really matter here, but I think it'll be easier to read with the first argument pulled to a variable.

GalLalouche · 2025-01-19T16:30:10Z

...c/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/nulls/CoalesceTests.java

+    }
+
+    /**
+     * Inserts random non-null garbage <strong>around</strong> the expected data


Javadoc doesn't not need to be 2 lines.

GalLalouche · 2025-01-19T16:31:04Z

...c/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/nulls/CoalesceTests.java

+        Block[] manyPositionsBlocks = new Block[Math.toIntExact(data.stream().filter(d -> d.isForceLiteral() == false).count())];
+        int realPosition = between(0, positions - 1);
+        try {
+            int b = 0;


Rename b to index or something?

Renaming. b means to me "the index variable for the array who's name stats with b", but of course, the array didn't start with b. I'll rename.

GalLalouche · 2025-01-19T16:32:16Z

...c/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/nulls/CoalesceTests.java

+                        if (p == realPosition) {
+                            builder.copyFrom(onePositionPage.getBlock(b), 0, 1);
+                        } else {
+                            builder.copyFrom(


There's way too much nesting here! Perhaps extracting this to a helper function would help readability?

This hadn't pushed me to my nesting limit, but if it has you I can flip it.

ivancea

Amazing. I wonder in how many places, we can do something like this. Something to be looking for for sure

ivancea · 2025-01-21T15:01:52Z

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/X-BlockBuilder.java.st

+     * </p>
+     */
+    @Override
+    public $Type$BlockBuilder copyFrom($Type$Block block, int position$if(BytesRef)$, BytesRef scratch$endif$) {


Should this be final? It looks like a dangerous performance regression if it was overridden at some point (I think?)

The class itself is final.

ivancea · 2025-01-21T15:04:58Z

.../org/elasticsearch/xpack/esql/expression/function/scalar/nulls/CoalesceBooleanEvaluator.java

Uhm, we don't have this project generated-src in .gitattributes, can you add it? 👀

nik9000 · 2025-01-23T15:35:54Z

Amazing. I wonder in how many places, we can do something like this. Something to be looking for for sure

Mostly CASE. Otherwise we get this by using normal evaluators most of the time.

``` before after (operation) Score Error Score Error Units coalesce_2_noop 75.949 ± 3.961 -> 0.010 ± 0.001 ns/op 99.9% coalesce_2_eager 99.299 ± 6.959 -> 4.292 ± 0.227 ns/op 95.7% coalesce_2_lazy 113.118 ± 5.747 -> 26.746 ± 0.954 ns/op 76.4% ``` We tend to advise folks that "COALESCE is faster than CASE", but, as of 8.16.0/elastic#112295 that wasn't the true. I was working with someone a few days ago to port a scripted_metric aggregation to ESQL and we saw COALESCE taking ~60% of the time. That won't do. The trouble is that CASE and COALESCE have to be *lazy*, meaning that operations like: ``` COALESCE(a, 1 / b) ``` should never emit a warning if `a` is not `null`, even if `b` is `0`. In 8.16/elastic#112295 CASE grew an optimization where it could operate non-lazily if it was flagged as "safe". This brings a similar optimization to COALESCE, see it above as "case_2_eager", a 95.7% improvement. It also brings and arguably more important optimization - entire-block execution for COALESCE. The schort version is that, if the first parameter of COALESCE returns no nulls we can return it without doing anything lazily. There are a few more cases, but the upshot is that COALESCE is pretty much *free* in cases where long strings of results are `null` or not `null`. That's the `coalesce_2_noop` line. Finally, when there mixed null and non-null values we were using a single builder with some fairly inefficient paths. This specializes them per type and skips some slow null-checking where possible. That's the `coalesce_2_lazy` result, a more modest 76.4%. NOTE: These %s of improvements on COALESCE itself, or COALESCE with some load-overhead operators like `+`. If COALESCE isn't taking a *ton* time in your query don't get particularly excited about this. It's fun though. Closes elastic#119953

elasticsearchmachine · 2025-01-23T17:41:29Z

💚 Backport successful

Status	Branch	Result
✅	8.x

``` before after (operation) Score Error Score Error Units coalesce_2_noop 75.949 ± 3.961 -> 0.010 ± 0.001 ns/op 99.9% coalesce_2_eager 99.299 ± 6.959 -> 4.292 ± 0.227 ns/op 95.7% coalesce_2_lazy 113.118 ± 5.747 -> 26.746 ± 0.954 ns/op 76.4% ``` We tend to advise folks that "COALESCE is faster than CASE", but, as of 8.16.0/#112295 that wasn't the true. I was working with someone a few days ago to port a scripted_metric aggregation to ESQL and we saw COALESCE taking ~60% of the time. That won't do. The trouble is that CASE and COALESCE have to be *lazy*, meaning that operations like: ``` COALESCE(a, 1 / b) ``` should never emit a warning if `a` is not `null`, even if `b` is `0`. In 8.16/#112295 CASE grew an optimization where it could operate non-lazily if it was flagged as "safe". This brings a similar optimization to COALESCE, see it above as "case_2_eager", a 95.7% improvement. It also brings and arguably more important optimization - entire-block execution for COALESCE. The schort version is that, if the first parameter of COALESCE returns no nulls we can return it without doing anything lazily. There are a few more cases, but the upshot is that COALESCE is pretty much *free* in cases where long strings of results are `null` or not `null`. That's the `coalesce_2_noop` line. Finally, when there mixed null and non-null values we were using a single builder with some fairly inefficient paths. This specializes them per type and skips some slow null-checking where possible. That's the `coalesce_2_lazy` result, a more modest 76.4%. NOTE: These %s of improvements on COALESCE itself, or COALESCE with some load-overhead operators like `+`. If COALESCE isn't taking a *ton* time in your query don't get particularly excited about this. It's fun though. Closes #119953

nik9000 added 7 commits January 13, 2025 15:15

ESQL: Speed COALESCE

edb6c03

WIP

8bd0989

Gotta make it nicer

759d052

COALESCE woo

f28f6af

Merge branch 'main' into rocket_coalesce

8dfed18

Formatting

40794d0

nik9000 added >non-issue :Analytics/ES|QL AKA ESQL v9.0.0 v8.18.0 labels Jan 14, 2025

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 14, 2025

nik9000 commented Jan 14, 2025

View reviewed changes

crespocarlos mentioned this pull request Jan 15, 2025

POC Service Map and Aggregated Critical Path with ESQL elastic/kibana#205596

Closed

Merge branch 'main' into rocket_coalesce

2615926

nik9000 added the auto-backport Automatically create backport pull requests when merged label Jan 16, 2025

nik9000 requested review from GalLalouche and ivancea January 17, 2025 17:07

GalLalouche approved these changes Jan 19, 2025

View reviewed changes

ivancea approved these changes Jan 22, 2025

View reviewed changes

Merge branch 'main' into rocket_coalesce

fbac0fe

nik9000 added 2 commits January 23, 2025 11:07

Update

8d81a3e

Fix

c93198d

nik9000 enabled auto-merge (squash) January 23, 2025 17:09

nik9000 merged commit dc4fa26 into elastic:main Jan 23, 2025
16 checks passed

nik9000 mentioned this pull request Jan 23, 2025

[8.x] Speed up COALESCE significantly (#120139) #120747

Merged

Speed up COALESCE significantly #120139

Speed up COALESCE significantly #120139

Uh oh!

Conversation

nik9000 commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jan 14, 2025

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!

GalLalouche 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!

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!

ivancea 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!

nik9000 commented Jan 23, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Jan 23, 2025

💚 Backport successful

Uh oh!

Uh oh!

nik9000 commented Jan 14, 2025 •

edited

Loading