-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
SQL: Fix qualified attribute resolution with CCS #81320
Conversation
In case the fields are qualified only by the index name in a cross cluster search query, the field resolution was failing since an attribute's qualification contains the cluster name, while a field's doesn't. This changes the existing `Attribute#qualifiedName()` to return a qualification including the index name only and adds (for completion) a new method, `fullyQualifiedName()`, which will return the 'cluster:index' qualification. This will allow queries like this to work: `SELECT remote_index.field FROM remote_cluster:remote_index` or `SELECT remote_index.field FROM remote_index` when the cluster is provided through the xDBC "cluster" parameter.
Pinging @elastic/es-ql (Team:QL) |
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.
Left some minor comments - LGTM otherwise.
import static org.elasticsearch.transport.RemoteClusterAware.buildRemoteIndexName; | ||
import static org.elasticsearch.xpack.ql.util.RemoteClusterUtils.splitQualifiedIndex; |
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.
Let's wrap this functionality (which is just string manipulation) inside StringUtils
inside ql - those methods can call just delegate behind the scenes.
RemoteClusterUtils
might be be folded into that if possible.
The upside is we don't depend directly on transport or remote write but rather it's just a StringUtils.splitCatalog
or something similar.
@@ -62,6 +74,10 @@ public String qualifiedName() { | |||
return qualifier == null ? name() : qualifier + "." + name(); | |||
} | |||
|
|||
public String fullyQualifiedName() { |
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.
Who uses fully qualified name? If there's nobody I would remove it.
If it has to stay, I recommend rename it something like catalogQualifiedName() - fully doesn't convey much.
- remove currently unused method; - move RemoteClusterUtils methods into StringUtils.
@elasticmachine run elasticsearch-ci/part-2 |
@elasticmachine update branch |
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.
LGTM
💚 Backport successful
|
In case the fields are qualified only by the index name in a cross cluster search query, the field resolution was failing since an attribute's qualification contains the cluster name, while a field's doesn't. This changes the existing `Attribute#qualifiedName()` to return a qualification including the index name only and adds (for completion) a new method, `fullyQualifiedName()`, which will return the 'cluster:index' qualification. This will allow queries like this to work: `SELECT remote_index.field FROM remote_cluster:remote_index` or `SELECT remote_index.field FROM remote_index` when the cluster is provided through the xDBC "cluster" parameter.
In case the fields are qualified only by the index name in a cross cluster search query, the field resolution was failing since an attribute's qualification contains the cluster name, while a field's doesn't. This changes the existing `Attribute#qualifiedName()` to return a qualification including the index name only and adds (for completion) a new method, `fullyQualifiedName()`, which will return the 'cluster:index' qualification. This will allow queries like this to work: `SELECT remote_index.field FROM remote_cluster:remote_index` or `SELECT remote_index.field FROM remote_index` when the cluster is provided through the xDBC "cluster" parameter.
In case the fields are qualified only by the index name in a
CCS SQL query, the field resolution was failing since an
attribute's qualification contains the cluster name, while a field's doesn't.
This changes the existing
Attribute#qualifiedName()
to return a qualificationincluding the index name only and adds (for completion) a new method,
fullyQualifiedName()
, which will return the 'cluster:index' qualification.This will allow queries like this to work:
SELECT remote_index.field FROM remote_cluster:remote_index
or
SELECT remote_index.field FROM remote_index
when the cluster is providedthrough the xDBC "cluster" parameter.