Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESQL: Fix EvalBenchmark #124736

Merged
merged 2 commits into from
Mar 14, 2025
Merged

ESQL: Fix EvalBenchmark #124736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b7e089
ESQL: Fix EvalBenchmark
nik9000 Mar 13, 2025
2cd6cb5
Merge branch 'main' into fix_eval_benchmark
nik9000 Mar 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions benchmarks/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ base {
archivesName = 'elasticsearch-benchmarks'
}

tasks.named("test").configure { enabled = false }
tasks.named("javadoc").configure { enabled = false }

configurations {
Expand All @@ -52,8 +51,10 @@ dependencies {
api "org.openjdk.jmh:jmh-core:$versions.jmh"
annotationProcessor "org.openjdk.jmh:jmh-generator-annprocess:$versions.jmh"
// Dependencies of JMH
runtimeOnly 'net.sf.jopt-simple:jopt-simple:5.0.4'
runtimeOnly 'net.sf.jopt-simple:jopt-simple:5.0.2'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this the tests don't start. It doesn't seem to hurt jmh. If it's trouble later we can deal with it.

runtimeOnly 'org.apache.commons:commons-math3:3.6.1'

testImplementation(project(':test:framework'))
}

// enable the JMH's BenchmarkProcessor to generate the final benchmark classes
Expand Down
10 changes: 10 additions & 0 deletions benchmarks/src/main/java/org/elasticsearch/benchmark/compute/operator/EvalBenchmark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

import org.apache.lucene.util.BytesRef;
import org.elasticsearch.common.breaker.NoopCircuitBreaker;
import org.elasticsearch.common.logging.LogConfigurator;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.compute.data.Block;
Expand All @@ -28,6 +29,8 @@
import org.elasticsearch.compute.operator.EvalOperator;
import org.elasticsearch.compute.operator.Operator;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.logging.LogManager;
import org.elasticsearch.logging.Logger;
import org.elasticsearch.xpack.esql.core.expression.Expression;
import org.elasticsearch.xpack.esql.core.expression.FieldAttribute;
import org.elasticsearch.xpack.esql.core.expression.FoldContext;
Expand Down Expand Up @@ -89,9 +92,16 @@ public class EvalBenchmark {
static final DriverContext driverContext = new DriverContext(BigArrays.NON_RECYCLING_INSTANCE, blockFactory);

static {
LogConfigurator.configureESLogging();
// Smoke test all the expected values and force loading subclasses more like prod
selfTest();
}

static void selfTest() {
Logger log = LogManager.getLogger(EvalBenchmark.class);
try {
for (String operation : EvalBenchmark.class.getField("operation").getAnnotationsByType(Param.class)[0].value()) {
log.info("self testing {}", operation);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to log something to make sure the tests really do something. I figured keeping it in the real benchmark wouldn't hurt. I did have to do the configureESLogging stuff to make it work though.

run(operation);
}
} catch (NoSuchFieldException e) {
Expand Down
18 changes: 18 additions & 0 deletions ...hmarks/src/test/java/org/elasticsearch/benchmark/compute/operator/EvalBenchmarkTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

package org.elasticsearch.benchmark.compute.operator;

import org.elasticsearch.test.ESTestCase;

public class EvalBenchmarkTests extends ESTestCase {
public void testSelfTest() {
EvalBenchmark.selfTest();
}
}
6 changes: 3 additions & 3 deletions .../elasticsearch/xpack/esql/expression/function/scalar/convert/AbstractConvertFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@

import joptsimple.internal.Strings;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.compute.data.Block;
import org.elasticsearch.compute.data.Page;
Expand All @@ -19,6 +17,8 @@
import org.elasticsearch.compute.operator.EvalOperator;
import org.elasticsearch.compute.operator.EvalOperator.ExpressionEvaluator;
import org.elasticsearch.compute.operator.Warnings;
import org.elasticsearch.logging.LogManager;
import org.elasticsearch.logging.Logger;
import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
import org.elasticsearch.xpack.esql.core.expression.Expression;
import org.elasticsearch.xpack.esql.core.tree.Source;
Expand Down Expand Up @@ -127,7 +127,7 @@ public ExpressionEvaluator.Factory toEvaluator(ToEvaluator toEvaluator) {

public abstract static class AbstractEvaluator implements EvalOperator.ExpressionEvaluator {

private static final Log logger = LogFactory.getLog(AbstractEvaluator.class);
private static final Logger logger = LogManager.getLogger(AbstractEvaluator.class);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual fix....


protected final DriverContext driverContext;
private final Warnings warnings;
Expand Down