-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL: Make field fusion generic #137382
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.
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
base: main
Are you sure you want to change the base?
ESQL: Make field fusion generic #137382
Changes from 24 commits
a3c526e
7758ab9
47c874e
6b0fead
1dd6d96
ce8e6aa
d50a74b
8746cfa
15eb5e9
d01184b
d6897d8
e091312
538b72b
2fc8fd5
e310d4b
bb81e43
bb9bca2
d70bca9
5fb351c
39ae15a
90005ca
b98935c
c462d07
667e58a
8626221
bd2e10e
bd9c9ab
06cb017
5e75a46
99392c6
dadb6c5
05af853
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| pr: 137382 | ||
| summary: Make field fusion generic | ||
| area: ES|QL | ||
| type: enhancement | ||
| issues: [] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /* | ||
| * 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.index.mapper.blockloader; | ||
|
|
||
| import org.elasticsearch.index.mapper.MappedFieldType; | ||
|
|
||
| /** | ||
| * Configuration needed to transform loaded values into blocks. | ||
| * {@link MappedFieldType}s will find me in | ||
| * {@link MappedFieldType.BlockLoaderContext#blockLoaderFunctionConfig()} and | ||
| * use this configuration to choose the appropriate implementation for | ||
| * transforming loaded values into blocks. | ||
| */ | ||
| public interface BlockLoaderFunctionConfig { | ||
| /** | ||
| * Name used in descriptions. | ||
| */ | ||
| String name(); | ||
|
|
||
| record Named(String name, Warnings warnings) implements BlockLoaderFunctionConfig { | ||
| @Override | ||
| public int hashCode() { | ||
|
||
| return name.hashCode(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (o == null || getClass() != o.getClass()) { | ||
| return false; | ||
| } | ||
| Named named = (Named) o; | ||
| return name.equals(named.name); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -252,6 +252,13 @@ public EsField getExactField() { | |
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Can this field be pushed <strong>if</strong> it is indexed? | ||
| */ | ||
| public boolean pushable() { | ||
| return true; | ||
| } | ||
|
||
|
|
||
| /** | ||
| * Returns and {@link Exact} object with all the necessary info about the field: | ||
| * <ul> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /* | ||
| * 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.expression.function; | ||
|
|
||
| import org.elasticsearch.compute.operator.DriverContext; | ||
| import org.elasticsearch.compute.operator.Warnings; | ||
| import org.elasticsearch.xpack.esql.core.tree.Source; | ||
|
|
||
| /** | ||
| * Shim between the {@link org.elasticsearch.index.mapper.blockloader.Warnings} in server and | ||
| * our {@link Warnings}. Also adds laziness because our {@link Warnings} are a little expensive | ||
| * on creation and {@link org.elasticsearch.index.mapper.blockloader.Warnings} wants to be | ||
| * cheap to create. | ||
| */ | ||
| public class BlockLoaderWarnings implements org.elasticsearch.index.mapper.blockloader.Warnings { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess these are warnings that can be created when using BlockLoader function config to load values. Should we add that to the javadoc? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| private final DriverContext.WarningsMode warningsMode; | ||
| private final Source source; | ||
| private Warnings delegate; | ||
|
|
||
| public BlockLoaderWarnings(DriverContext.WarningsMode warningsMode, Source source) { | ||
| this.warningsMode = warningsMode; | ||
| this.source = source; | ||
| } | ||
|
|
||
| @Override | ||
| public void registerException(Class<? extends Exception> exceptionClass, String message) { | ||
| if (delegate == null) { | ||
| delegate = Warnings.createOnlyWarnings( | ||
| warningsMode, | ||
| source.source().getLineNumber(), | ||
| source.source().getColumnNumber(), | ||
| source.text() | ||
| ); | ||
| } | ||
| delegate.registerException(exceptionClass, message); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we have an enum for this? We should avoid using strings. If too hard, at least make it a public static final String.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't really a limit to what we might push. We could make an enum in
serverwith everything which would be ok, but there will be cases where the field that supports the push is outside of server too. So the only change in server would be the enum. That feels... wrong. Not sure it's worse than using a strong though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What prevents us adding to the enum every time we push something new?
I thought this is not about the field, but about the name of the Function we are pushing? In this case the function name is LENGTH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do if you like the idea. I think we'll want to switch it away from an enum - but it can wait until it's obvious. And maybe it'll just stay an enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be OK with
public static final Stringtoo, especially since you use it in two different places in this same file.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, can't you just ask the blContext.blockLoaderFunctionConfig() to give you the appropriate BlockLoader and use inheritance? Then you don't even need case statement.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to generalize this. If we support 50 pushable functions in the future, do we really want a case statement with 50 cases here? Probably no. So inheritance seems best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed the
enumthis morning.The problem is this is dynamic dispatch based on a combination of the function and the field type and the field's configuration. I think maybe let's go with the
enumfor a while and replace it when it gets frustrating. We're not serializing here so we can refactor this any time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I hadn't read your comment about the ineritance-like approach. I don't particularly like it better than the
enums though.