Skip to content

ESQL: Fix Attribute#withQualifier ignoring the new qualifier - #152921

Merged
alex-spies merged 3 commits into
elastic:mainfrom
SEPURI-SAI-KRISHNA:esql-fix-with-qualifier
Aug 7, 2026
Merged

ESQL: Fix Attribute#withQualifier ignoring the new qualifier#152921
alex-spies merged 3 commits into
elastic:mainfrom
SEPURI-SAI-KRISHNA:esql-fix-with-qualifier

Conversation

@SEPURI-SAI-KRISHNA

@SEPURI-SAI-KRISHNA SEPURI-SAI-KRISHNA commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Fixes #152920

Attribute.withQualifier(String qualifier) compared the method parameter to
itself (Objects.equals(qualifier, qualifier) — the parameter shadows the
field of the same name), which is always true. As a result the method
unconditionally returned this and silently never applied the new qualifier.

All sibling methods (withName, withLocation, withDataType) compare the
accessor against the parameter; this change makes withQualifier do the same:

return Objects.equals(qualifier(), qualifier)
    ? this
    : clone(source(), qualifier, name(), dataType(), nullable(), id(), synthetic());

There are no callers of withQualifier in main yet, so there is no
user-visible impact today — but qualifiers in attributes are under active
development (esql_qualifiers_in_attributes), and the first caller would have
silently received the original, unqualified attribute back.

Also adds a regression test (ReferenceAttributeTests#testWithQualifier)
asserting that the new qualifier is applied and that passing an equal
qualifier still returns the same instance without cloning.

Verified locally with :x-pack:plugin:esql:test --tests "...ReferenceAttributeTests"
(9/9 passing) and :x-pack:plugin:esql:spotlessJavaCheck.

@elasticsearchmachine elasticsearchmachine added v9.5.0 needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jul 4, 2026
SEPURI-SAI-KRISHNA added a commit to SEPURI-SAI-KRISHNA/elasticsearch that referenced this pull request Jul 4, 2026
@SEPURI-SAI-KRISHNA
Add changelog for elastic#152921
d32cf8b
@Kubik42 Kubik42 added >non-issue :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels Jul 9, 2026
@elasticsearchmachine

elasticsearchmachine commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 9, 2026
@wchaparro

wchaparro commented Aug 5, 2026

Copy link
Copy Markdown
Member

Hi @SEPURI-SAI-KRISHNA — this PR has been identified as stalled (open and without meaningful updates for 30+ days).

Please update the PR or confirm its status when you can.

It will be closed in two weeks if there is no further activity. Please reply, push an update, or close it yourself if it is no longer needed.

— Analytics PR review management

@SEPURI-SAI-KRISHNA
ESQL: Fix no-op Attribute#withQualifier
ba5cf78 
The qualifier parameter shadowed the field, so
  Objects.equals(qualifier, qualifier) compared the parameter to itself
  and the method always returned this, never applying the new qualifier.
  Compare against qualifier() like the sibling with* methods do, and add
  a regression test.

  Fixes elastic#152920
@SEPURI-SAI-KRISHNA
Add changelog for elastic#152921
8c259a7
@SEPURI-SAI-KRISHNA

SEPURI-SAI-KRISHNA commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Friendly ping — this picked up the stalled label, but I don't think there are open questions on it; it looks like it just never got a reviewer assigned (there's no CODEOWNERS entry
covering x-pack/plugin/esql, so no team was auto-requested). I've just rebased onto current main.

The bug is still present on main: Attribute#withQualifier compares Objects.equals(qualifier, qualifier), where the parameter shadows the field, so the guard is unconditionally true
and the method always returns this. Every sibling — withName, withDataType, withNullability, withLocation — compares accessor-against-parameter; this one-line change makes
withQualifier match that pattern, plus a regression test in ReferenceAttributeTests.

There are no callers on main today, so there's no user-visible impact — but it will silently no-op for the qualifier work in flight.

@elastic/es-analytical-engine could someone take a look, and kick off CI when you get a chance? cc @bpintea

@bpintea

bpintea commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

buildkite test this

@bpintea bpintea self-assigned this Aug 6, 2026
@bpintea bpintea added v9.5.1 v9.4.6 auto-backport Automatically create backport pull requests when merged >bug and removed stalled labels Aug 6, 2026
@bpintea

bpintea commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@alex-spies, the proposed solution here is correct, but the fixed method is currently dead code. Will we needed it later for the qualifiers work? Otherwise we might want to instead remove it.

@SEPURI-SAI-KRISHNA

SEPURI-SAI-KRISHNA commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review and for kicking off CI, @bpintea.

One data point that may help the keep-vs-remove call: this method is a port of xpack.ql.expression.Attribute#withQualifier (x-pack/plugin/ql/.../Attribute.java:89), where the comparison is correct and the method has live callers — sql Analyzer:1086 and SubQueryAlias:47, eql OptionalResolvedAttribute:65, and ql FieldAttribute:134. So the shadowing looks like it was introduced when the class was forked into esql-core, rather than being an intentionally unused stub.

Confirmed the only caller in ES|QL today is the test added here. Happy either way — if you'd prefer removal, I can swap this PR to delete the method instead; just say which and I'll push.

@alex-spies alex-spies assigned bpintea and unassigned bpintea Aug 6, 2026
@alex-spies alex-spies mentioned this pull request Aug 6, 2026
Closed
alex-spies
alex-spies approved these changes Aug 6, 2026
View reviewed changes

@alex-spies alex-spies left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@alex-spies, the proposed solution here is correct, but the fixed method is currently dead code. Will we needed it later for the qualifiers work? Otherwise we might want to instead remove it.

Removing and fixing are both fine by me. Qualifiers are still planned at some point, so fixing is fine IMHO.

Let's just get this in, as this LGTM.

But let's not backport this, it's not a user-facing bug at all. For the same reason, I'll go and strip the changelog, given that the changelog is for changes in features, behavior etc. that users can experience.

@alex-spies alex-spies removed >bug auto-backport Automatically create backport pull requests when merged v9.4.6 v9.5.2 labels Aug 6, 2026
@alex-spies
Delete docs/changelog/152921.yaml
b49b517 
Not a user-facing bug, no changelog needed.
@alex-spies

alex-spies commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Thanks for the contribution @SEPURI-SAI-KRISHNA , and thanks for the review @bpintea !

@alex-spies
alex-spies enabled auto-merge (squash) August 6, 2026 16:36
@alex-spies
alex-spies disabled auto-merge August 7, 2026 07:16
@alex-spies
alex-spies merged commit 9eb603a into elastic:main Aug 7, 2026
43 of 44 checks passed
@SEPURI-SAI-KRISHNA

SEPURI-SAI-KRISHNA commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Thank you @alex-spies and @bpintea

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

Reviewers

@alex-spies alex-spies alex-spies approved these changes

Assignees

@bpintea bpintea

Labels

:Analytics/ES|QL AKA ESQL external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.6.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ES|QL: Attribute.withQualifier() is a no-op due to self-comparison

6 participants

@SEPURI-SAI-KRISHNA @elasticsearchmachine @wchaparro @bpintea @alex-spies @Kubik42
Sponsor
SponsoredKunjungi sekarang
Promo