Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
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

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
9191000
2 changes: 1 addition & 1 deletion server/src/main/resources/transport/upper_bounds/9.3.csv
Original file line number Diff line number Diff line change
@@ -1 +1 @@
reordered_translog_operations,9190000
esql_configuration_query_settings,9191000
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,13 @@
import org.elasticsearch.xpack.esql.analysis.PreAnalyzer;
import org.elasticsearch.xpack.esql.analysis.Verifier;
import org.elasticsearch.xpack.esql.common.Failures;
import org.elasticsearch.xpack.esql.core.expression.FoldContext;
import org.elasticsearch.xpack.esql.enrich.EnrichPolicyResolver;
import org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry;
import org.elasticsearch.xpack.esql.optimizer.LogicalOptimizerContext;
import org.elasticsearch.xpack.esql.optimizer.LogicalPlanOptimizer;
import org.elasticsearch.xpack.esql.optimizer.LogicalPlanPreOptimizer;
import org.elasticsearch.xpack.esql.optimizer.LogicalPreOptimizerContext;
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
import org.elasticsearch.xpack.esql.planner.mapper.Mapper;
import org.elasticsearch.xpack.esql.plugin.EsqlQueryClusterSettings;
import org.elasticsearch.xpack.esql.plugin.TransportActionServices;
import org.elasticsearch.xpack.esql.querylog.EsqlQueryLog;
import org.elasticsearch.xpack.esql.session.Configuration;
import org.elasticsearch.xpack.esql.session.EsqlSession;
import org.elasticsearch.xpack.esql.session.IndexResolver;
import org.elasticsearch.xpack.esql.session.Result;
Expand Down Expand Up @@ -72,8 +67,7 @@ public PlanExecutor(
public void esql(
EsqlQueryRequest request,
String sessionId,
Configuration cfg,
FoldContext foldContext,
EsqlQueryClusterSettings esqlQueryClusterSettings,
EnrichPolicyResolver enrichPolicyResolver,
EsqlExecutionInfo executionInfo,
IndicesExpressionGrouper indicesExpressionGrouper,
Expand All @@ -84,13 +78,11 @@ public void esql(
final PlanTelemetry planTelemetry = new PlanTelemetry(functionRegistry);
final var session = new EsqlSession(
sessionId,
cfg,
esqlQueryClusterSettings,
indexResolver,
enrichPolicyResolver,
preAnalyzer,
new LogicalPlanPreOptimizer(new LogicalPreOptimizerContext(foldContext, services.inferenceService())),
functionRegistry,
new LogicalPlanOptimizer(new LogicalOptimizerContext(cfg, foldContext)),
mapper,
verifier,
planTelemetry,
Expand Down
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "Configuration" object wasn't being used here, at all. So I removed it. And this led to many changes along tests.
That's good btw, as otherwise creating the config after parsing would have been quite harder

Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry;
import org.elasticsearch.xpack.esql.plan.EsqlStatement;
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
import org.elasticsearch.xpack.esql.session.Configuration;
import org.elasticsearch.xpack.esql.telemetry.PlanTelemetry;

import java.util.BitSet;
Expand Down Expand Up @@ -100,41 +99,40 @@ public void setEsqlConfig(EsqlConfig config) {
}

// testing utility
public LogicalPlan createStatement(String query, Configuration configuration) {
return createStatement(query, new QueryParams(), configuration);
public LogicalPlan createStatement(String query) {
return createStatement(query, new QueryParams());
}

// testing utility
public LogicalPlan createStatement(String query, QueryParams params, Configuration configuration) {
return createStatement(query, params, new PlanTelemetry(new EsqlFunctionRegistry()), configuration);
public LogicalPlan createStatement(String query, QueryParams params) {
return createStatement(query, params, new PlanTelemetry(new EsqlFunctionRegistry()));
}

public LogicalPlan createStatement(String query, QueryParams params, PlanTelemetry metrics, Configuration configuration) {
public LogicalPlan createStatement(String query, QueryParams params, PlanTelemetry metrics) {
if (log.isDebugEnabled()) {
log.debug("Parsing as statement: {}", query);
}
return invokeParser(query, params, metrics, EsqlBaseParser::singleStatement, AstBuilder::plan, configuration);
return invokeParser(query, params, metrics, EsqlBaseParser::singleStatement, AstBuilder::plan);
}

// testing utility
public EsqlStatement createQuery(String query, QueryParams params, Configuration configuration) {
return createQuery(query, params, new PlanTelemetry(new EsqlFunctionRegistry()), configuration);
public EsqlStatement createQuery(String query, QueryParams params) {
return createQuery(query, params, new PlanTelemetry(new EsqlFunctionRegistry()));
}

public EsqlStatement createQuery(String query, QueryParams params, PlanTelemetry metrics, Configuration configuration) {
public EsqlStatement createQuery(String query, QueryParams params, PlanTelemetry metrics) {
if (log.isDebugEnabled()) {
log.debug("Parsing as statement: {}", query);
}
return invokeParser(query, params, metrics, EsqlBaseParser::statements, AstBuilder::statement, configuration);
return invokeParser(query, params, metrics, EsqlBaseParser::statements, AstBuilder::statement);
}

private <T> T invokeParser(
String query,
QueryParams params,
PlanTelemetry metrics,
Function<EsqlBaseParser, ParserRuleContext> parseFunction,
BiFunction<AstBuilder, ParserRuleContext, T> result,
Configuration configuration
BiFunction<AstBuilder, ParserRuleContext, T> result
) {
if (query.length() > MAX_LENGTH) {
throw new ParsingException("ESQL statement is too large [{} characters > {}]", query.length(), MAX_LENGTH);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,96 +7,74 @@

package org.elasticsearch.xpack.esql.plan;

import org.elasticsearch.TransportVersion;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.transport.RemoteClusterService;
import org.elasticsearch.xpack.esql.core.QlIllegalArgumentException;
import org.elasticsearch.xpack.esql.core.expression.Expression;
import org.elasticsearch.xpack.esql.core.type.DataType;
import org.elasticsearch.xpack.esql.expression.Foldables;
import org.elasticsearch.xpack.esql.parser.ParsingException;

import java.util.function.Predicate;
import java.io.IOException;
import java.time.ZoneId;
import java.time.ZoneOffset;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Function;

public enum QuerySettings {
public class QuerySettings {
// TODO check cluster state and see if project routing is allowed
// see https://github.com/elastic/elasticsearch/pull/134446
// PROJECT_ROUTING(..., state -> state.getRemoteClusterNames().crossProjectEnabled());
PROJECT_ROUTING(
public static final QuerySettingDef<String> PROJECT_ROUTING = new QuerySettingDef<>(
"project_routing",
DataType.KEYWORD,
true,
false,
true,
"A project routing expression, "
+ "used to define which projects to route the query to. "
+ "Only supported if Cross-Project Search is enabled."
),;

private String settingName;
private DataType type;
private final boolean serverlessOnly;
private final boolean snapshotOnly;
private final boolean preview;
private final String description;
private final Predicate<RemoteClusterService> validator;

QuerySettings(
String name,
DataType type,
boolean serverlessOnly,
boolean preview,
boolean snapshotOnly,
String description,
Predicate<RemoteClusterService> validator
) {
this.settingName = name;
this.type = type;
this.serverlessOnly = serverlessOnly;
this.preview = preview;
this.snapshotOnly = snapshotOnly;
this.description = description;
this.validator = validator;
}

QuerySettings(String name, DataType type, boolean serverlessOnly, boolean preview, boolean snapshotOnly, String description) {
this(name, type, serverlessOnly, preview, snapshotOnly, description, state -> true);
}

public String settingName() {
return settingName;
}

public DataType type() {
return type;
}

public boolean serverlessOnly() {
return serverlessOnly;
}
+ "Only supported if Cross-Project Search is enabled.",
(value, settings) -> Foldables.stringLiteralValueOf(value, "Unexpected value"),
(_rcs) -> null
);

public boolean snapshotOnly() {
return snapshotOnly;
}

public boolean preview() {
return preview;
}

public String description() {
return description;
}
public static final QuerySettingDef<ZoneId> TIME_ZONE = new QuerySettingDef<>(
"time_zone",
DataType.KEYWORD,
false,
true,
true,
"The default timezone to be used in the query, by the functions and commands that require it. Defaults to UTC",
(value, _rcs) -> {
String timeZone = Foldables.stringLiteralValueOf(value, "Unexpected value");
try {
return ZoneId.of(timeZone);
} catch (Exception exc) {
throw new QlIllegalArgumentException("Invalid time zone [" + timeZone + "]");
}
},
(_rcs) -> ZoneOffset.UTC
);

public Predicate<RemoteClusterService> validator() {
return validator;
}
public static final QuerySettingDef<?>[] ALL_SETTINGS = { PROJECT_ROUTING, TIME_ZONE };

public static void validate(EsqlStatement statement, RemoteClusterService clusterService) {
for (QuerySetting setting : statement.settings()) {
boolean found = false;
for (QuerySettings qs : values()) {
if (qs.settingName().equals(setting.name())) {
for (QuerySettingDef<?> def : ALL_SETTINGS) {
if (def.name().equals(setting.name())) {
found = true;
if (setting.value().dataType() != qs.type()) {
throw new ParsingException(setting.source(), "Setting [" + setting.name() + "] must be of type " + qs.type());
if (setting.value().dataType() != def.type()) {
throw new ParsingException(setting.source(), "Setting [" + setting.name() + "] must be of type " + def.type());
}
if (qs.validator().test(clusterService) == false) {
throw new ParsingException(setting.source(), "Setting [" + setting.name() + "] is not allowed");
String error = def.validator().validate(setting.value(), clusterService);
if (error != null) {
throw new ParsingException("Error validating setting [" + setting.name() + "]: " + error);
}
break;
}
Expand All @@ -106,4 +84,118 @@ public static void validate(EsqlStatement statement, RemoteClusterService cluste
}
}
}

public static QuerySettingsMap toMap(EsqlStatement statement) {
var settings = new HashMap<String, Expression>();
for (QuerySetting setting : statement.settings()) {
settings.put(setting.name(), setting.value());
}
return new QuerySettingsMap(settings);
}

public static class QuerySettingsMap implements Writeable {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The QuerySettings class has become a class without instances. Why not re-use it for the map, rather than having a separate class that holds the map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'll try to do that (in the next PR)

private static final TransportVersion ESQL_CONFIGURATION_QUERY_SETTINGS = TransportVersion.fromName(
"esql_configuration_query_settings"
);

private final Map<String, Expression> settings;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd be a little more relaxed if this was restricted to Literals rather than arbitrary expressions ^^" Having an expression as setting sounds more complex than we currently need.


public QuerySettingsMap(Map<String, Expression> settings) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Any reason for this not being a Map<QuerySettingsDef, Expression>? We could back this with an enum map to avoid looking things up by string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently the Def has validation, parsing... It's similar to Settings, but for ESQL.
I created this class just to be transported to datanodes: Plain data, to be read by datanodes using their Defs.

As I'm removing transport for this PR, I'll rethink a bit those topics

Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine and the API you made is cool.

But, maybe looking up by string will be a little heavy; the querysettingsdefs are kinda just an enum and could be assigned ids as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could store it in the configuration later as the map you comment yes. Even without IDs, as they're static objects. It would matter at transport time though, but we're free to serialize as strings in there if we want.

So yep, I'll make this change in the next PR!

this.settings = settings;
}

public QuerySettingsMap(StreamInput in) throws IOException {
if (in.getTransportVersion().supports(ESQL_CONFIGURATION_QUERY_SETTINGS)) {
this.settings = in.readMap(StreamInput::readString, r -> r.readNamedWriteable(Expression.class));
} else {
this.settings = Map.of();
}
}

@Override
public void writeTo(StreamOutput out) throws IOException {
if (out.getTransportVersion().supports(ESQL_CONFIGURATION_QUERY_SETTINGS)) {
out.writeMap(settings, StreamOutput::writeString, StreamOutput::writeNamedWriteable);
}
}

public <T> T get(QuerySettingDef<T> def, RemoteClusterService clusterService) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Having to pass in the RCS every time sounds wrong. Isn't this something that we'd only do once, when the query is first parsed and we read the QuerySetting values from the parsed query?

That is, I'd expect that we use the RCS once while building the QuerySettingsMap, and after, the settings are fixed and passed around.

Otherwise, we'd have to pass the RCS to all functions that might want to get a setting value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the same. Theoretically, it will depend on how we store this in the config: If we store the original Expression, or the final value. This is still a bit abstract, as no actual config uses that yet though, so I'm not sure if this is required here to begin with. I could remove it from get() and keep it only in validate().
@luigidellaquila, some idea of what we need that for?

Copy link
Contributor

Choose a reason for hiding this comment

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

or the final value
I think we should only store the final (Literal or <T>) value unless there's a good reason for settings to be complex enough that they have a dynamic value that can change where the setting is used. But even then, I think it still should be a final value and whatever is dynamic about it should be retrieved on the node where it's relevant.

I think the flow should be

  1. raw parsing from the query string
  2. validate + parse the actual settings, use RCS or pass in whatever else the settings need to be validated or parsed
  3. store the parsed result in a map that is passed on as part of the config.

Expression value = settings.get(def.name());
if (value == null) {
return def.defaultValueSupplier.apply(clusterService);
}
return def.get(value, clusterService);
}
}

/**
* Definition of a query setting.
*
* @param name The name to be used when setting it in the query. E.g. {@code SET name=value}
* @param type The allowed datatype of the setting.
* @param serverlessOnly
* @param preview
* @param snapshotOnly
* @param description The user-facing description of the setting.
* @param validator A validation function to check the setting value.
* Defaults to calling the {@link #parser} and returning the error message of any exception it throws.
* @param parser A function to parse the setting value into the final object.
* @param defaultValueSupplier A supplier of the default value to be used when the setting is not set.
* @param <T> The type of the setting value.
*/
public record QuerySettingDef<T>(
String name,
DataType type,
boolean serverlessOnly,
boolean preview,
boolean snapshotOnly,
String description,
Validator validator,
Parser<T> parser,
Function<RemoteClusterService, T> defaultValueSupplier
) {
public QuerySettingDef(
String name,
DataType type,
boolean serverlessOnly,
boolean preview,
boolean snapshotOnly,
String description,
Parser<T> parser,
Function<RemoteClusterService, T> defaultValueSupplier
) {
this(name, type, serverlessOnly, preview, snapshotOnly, description, (value, rcs) -> {
try {
parser.parse(value, rcs);
return null;
} catch (Exception exc) {
return exc.getMessage();
}
}, parser, defaultValueSupplier);
}

public T get(Expression value, RemoteClusterService clusterService) {
if (value == null) {
return defaultValueSupplier.apply(clusterService);
}
return parser.parse(value, clusterService);
}

@FunctionalInterface
public interface Validator {
/**
* Validates the setting value and returns the error message if there's an error, or null otherwise.
*/
@Nullable
String validate(Expression value, RemoteClusterService clusterService);
}

@FunctionalInterface
public interface Parser<T> {
/**
* Parses an already validated expression.
*/
T parse(Expression value, RemoteClusterService clusterService);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* 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; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

package org.elasticsearch.xpack.esql.plugin;

public record EsqlQueryClusterSettings(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a bundle of the required cluster settings used by the EsqlSession to create the configuration. We could also pass the cluster settings themselves and let the EsqlSession do it itself, but I wasn't sure if we wanted that. I'll evaluate the change

int resultTruncationMaxSize,
int resultTruncationDefaultSize,
int timeseriesResultTruncationMaxSize,
int timeseriesResultTruncationDefaultSize
) {}
Loading
Loading