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.

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: standardize writeTo method across Functions #118037

Open
costin opened this issue Dec 5, 2024 · 2 comments
Open

ESQL: standardize writeTo method across Functions #118037

costin opened this issue Dec 5, 2024 · 2 comments
Assignees
Labels
:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)

Comments

@costin
Copy link
Member

costin commented Dec 5, 2024

Description

The vast majority of ESQL functions follow the same pattern, accepting one or two fields and thus end up with the same writeTo method:

    public void writeTo(StreamOutput out) throws IOException {
        Source.EMPTY.writeTo(out);
        for (Expression child : arguments()) {
              out.writeNamedWritable(child);
        }
    }

Yet because the method is not already implemented, a lot of functions end up implementing this method, causing small variants.
This was partially addressed in AggregateFunction however the approach should be generalized to all Functions by:
a. implementing a default final writeTo method
b. (optionally) provide an empty extraWriteTo method (or similar) to allow extra payload to be written - though we'd have to identify a case for this first.

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Dec 5, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@idegtiarenko
Copy link
Contributor

idegtiarenko commented Dec 5, 2024

This should also somehow handle optional arguments in Substring, DateFormat, DateParse and possibly other implementations.
Such arguments are currently written with writeOptionalNamedWriteable. This needs to be preserved if we want to avoid transport change:

) {
super(source, length == null ? Arrays.asList(str, start) : Arrays.asList(str, start, length));
this.str = str;
this.start = start;
this.length = length;
}
private Substring(StreamInput in) throws IOException {
this(
Source.readFrom((PlanStreamInput) in),
in.readNamedWriteable(Expression.class),
in.readNamedWriteable(Expression.class),
in.readOptionalNamedWriteable(Expression.class)
);
}
@Override
public void writeTo(StreamOutput out) throws IOException {
source().writeTo(out);
out.writeNamedWriteable(str);
out.writeNamedWriteable(start);
out.writeOptionalNamedWriteable(length);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)
Projects
None yet
Development

No branches or pull requests

3 participants