Skip to content

Conversation

@not-napoleon
Copy link
Member

FieldAttribute had seven constructors. This PR removes three of them, and replaces them with a more readable builder pattern.

@not-napoleon not-napoleon requested review from alex-spies and nik9000 May 7, 2025 18:46
@not-napoleon not-napoleon added >non-issue auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.19.0 v9.1.0 labels May 7, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 7, 2025

private static FieldAttribute intField() {
return new FieldAttribute(Source.EMPTY, "int", new EsField("int", DataType.INTEGER, Map.of(), true));
return new FieldAttribute.FieldAttirbuteBuilder(Source.EMPTY, "int", new EsField("int", DataType.INTEGER, Map.of(), true)).build();
Copy link
Member

Choose a reason for hiding this comment

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

Spelling.

Maybe should just be FieldAttribute.builder() or new FieldAttribute.Builder()?

protected PhysicalPlan rule(EsSourceExec plan) {
var docId = new FieldAttribute(plan.source(), EsQueryExec.DOC_ID_FIELD.getName(), EsQueryExec.DOC_ID_FIELD);
var docId = new FieldAttribute.FieldAttirbuteBuilder(plan.source(), EsQueryExec.DOC_ID_FIELD.getName(), EsQueryExec.DOC_ID_FIELD)
.build();
Copy link
Contributor

@idegtiarenko idegtiarenko May 7, 2025

Choose a reason for hiding this comment

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

There are several cases when the builder is used like here: Builder(...).build() without extra chain calls.
I wonder if it is possible to still keep a constructor with such signature? This will reduce amount of objects created in prod code (not sure if escape analysis proactively eliminates them).

I also wonder if we could achieve similar level of simplification if we:

Copy link
Member Author

Choose a reason for hiding this comment

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

remove constructors with fewer usages ans supply extra arguments explicitly

I was under the impression that FieldAttribute, being a sub-class of Node was meant to be immutable. I'm not opposed to adding some setters on it for these optional fields, but if immutability is important for these classes, that seems like a risky plan

Copy link
Member

Choose a reason for hiding this comment

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

I'd keep it immutable. We certainly could just hard call the ctor every time if we're worried about the allocations, but I haven't been too worried about them in the past.

Copy link
Contributor

Choose a reason for hiding this comment

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

remove constructors with fewer usages and supply extra arguments explicitly

To be precise, supply default argument explicitly via constructor prior constructor was delegating to

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 wrote up what I think you're asking for in #127920. Personally, I like the builder better, but I'm open to feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.19.0 v9.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants