Skip to content

Commit 96ffaf0

Browse files
authored
ESQL: Remove extra block calls in evaluators and aggregators (#135919)
2 implementers changed: - Aggregators: Change `isNull() + getValueCount()` to a single `getValueCount()`. `getValueCount()` delegates to `isNull()` internally for Array blocks, and they're supposed to be in sync. - Evaluators: Same change, also removing an impossible branch (`getValueCount() == 0` after the `isNull() == false`) These changes were made to: - Improve readability/branch sanity, specially in the evaluators case. - Slight performance improvement. itable accesses have been problematic in similar cases, and this change removes up to 25% of them. There are 2 cases: - The position is null: We change a `isNull()` to a `getValueCount()` delegating to `isNull()` in some cases - The position is not null: We change a `isNull() + getValueCount()` by removing the `isNull()`, which could access a `BitSet` Aggs microbenchmarks show no differences.
1 parent d02d828 commit 96ffaf0

File tree

321 files changed

+4127
-3954
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

321 files changed

+4127
-3954
lines changed

x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/AggregatorImplementer.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -428,7 +428,8 @@ private MethodSpec addRawBlock(boolean masked) {
428428
builder.beginControlFlow("if (mask.getBoolean(p) == false)").addStatement("continue").endControlFlow();
429429
}
430430
for (AggregationParameter p : aggParams) {
431-
builder.beginControlFlow("if ($L.isNull(p))", p.blockName());
431+
builder.addStatement("int $LValueCount = $L.getValueCount(p)", p.name(), p.blockName());
432+
builder.beginControlFlow("if ($LValueCount == 0)", p.name());
432433
builder.addStatement("continue");
433434
builder.endControlFlow();
434435
}
@@ -447,7 +448,7 @@ private MethodSpec addRawBlock(boolean masked) {
447448
}
448449
for (AggregationParameter p : aggParams) {
449450
builder.addStatement("int $L = $L.getFirstValueIndex(p)", p.startName(), p.blockName());
450-
builder.addStatement("int $L = $L + $L.getValueCount(p)", p.endName(), p.startName(), p.blockName());
451+
builder.addStatement("int $L = $L + $LValueCount", p.endName(), p.startName(), p.name());
451452
builder.beginControlFlow(
452453
"for (int $L = $L; $L < $L; $L++)",
453454
p.offsetName(),

x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/argument/StandardArgument.java

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -148,25 +148,23 @@ public void sumBaseRamBytesUsed(MethodSpec.Builder builder) {
148148
}
149149

150150
static void skipNull(MethodSpec.Builder builder, String value) {
151-
builder.beginControlFlow("if ($N.isNull(p))", value);
151+
builder.beginControlFlow("switch ($N.getValueCount(p))", value);
152152
{
153-
builder.addStatement("result.appendNull()");
154-
builder.addStatement("continue position");
155-
}
156-
builder.endControlFlow();
157-
builder.beginControlFlow("if ($N.getValueCount(p) != 1)", value);
158-
{
159-
builder.beginControlFlow("if ($N.getValueCount(p) > 1)", value);
160-
{
161-
builder.addStatement(
162-
// TODO: reflection on SingleValueQuery.MULTI_VALUE_WARNING?
163-
"warnings().registerException(new $T(\"single-value function encountered multi-value\"))",
164-
IllegalArgumentException.class
165-
);
166-
}
167-
builder.endControlFlow();
168-
builder.addStatement("result.appendNull()");
169-
builder.addStatement("continue position");
153+
builder.addCode("case 0:\n");
154+
builder.addStatement(" result.appendNull()");
155+
builder.addStatement(" continue position");
156+
157+
builder.addCode("case 1:\n");
158+
builder.addStatement(" break");
159+
160+
builder.addCode("default:\n");
161+
builder.addStatement(
162+
// TODO: try to use SingleValueQuery.MULTI_VALUE_WARNING?
163+
" warnings().registerException(new $T(\"single-value function encountered multi-value\"))",
164+
IllegalArgumentException.class
165+
);
166+
builder.addStatement(" result.appendNull()");
167+
builder.addStatement(" continue position");
170168
}
171169
builder.endControlFlow();
172170
}

x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/CountDistinctBooleanAggregatorFunction.java

Lines changed: 6 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/CountDistinctBytesRefAggregatorFunction.java

Lines changed: 6 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/CountDistinctDoubleAggregatorFunction.java

Lines changed: 6 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/CountDistinctFloatAggregatorFunction.java

Lines changed: 6 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/CountDistinctIntAggregatorFunction.java

Lines changed: 6 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/CountDistinctLongAggregatorFunction.java

Lines changed: 6 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/FirstBytesRefByTimestampAggregatorFunction.java

Lines changed: 12 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/FirstDoubleByTimestampAggregatorFunction.java

Lines changed: 12 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)