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

SQL: Fix qualified attribute resolution with CCS #81320

Merged
merged 5 commits into from
Dec 9, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix qualified attribute resolution with CCS
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.
  • Loading branch information
bpintea committed Dec 3, 2021
commit 2b33956c5a80435133b3063eb7f4bf8c6a32f3ec
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,16 @@
*/
package org.elasticsearch.xpack.ql.expression;

import org.elasticsearch.core.Tuple;
import org.elasticsearch.xpack.ql.tree.Source;
import org.elasticsearch.xpack.ql.type.DataType;

import java.util.List;
import java.util.Objects;

import static java.util.Collections.emptyList;
import static org.elasticsearch.transport.RemoteClusterAware.buildRemoteIndexName;
import static org.elasticsearch.xpack.ql.util.RemoteClusterUtils.splitQualifiedIndex;
Copy link
Member

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.


/**
* {@link Expression}s that can be materialized and describe properties of the derived table.
Expand All @@ -31,6 +34,8 @@ public abstract class Attribute extends NamedExpression {
// empty - such as a top level attribute in SELECT cause
// present - table name or a table name alias
private final String qualifier;
// cluster name in the qualifier (if any)
private final String cluster;

// can the attr be null - typically used in JOINs
private final Nullability nullability;
Expand All @@ -45,7 +50,14 @@ public Attribute(Source source, String name, String qualifier, Nullability nulla

public Attribute(Source source, String name, String qualifier, Nullability nullability, NameId id, boolean synthetic) {
super(source, name, emptyList(), id, synthetic);
this.qualifier = qualifier;
if (qualifier != null) {
Tuple<String, String> splitQualifier = splitQualifiedIndex(qualifier);
this.cluster = splitQualifier.v1();
this.qualifier = splitQualifier.v2();
} else {
this.cluster = null;
this.qualifier = null;
}
this.nullability = nullability;
}

Expand All @@ -62,6 +74,10 @@ public String qualifiedName() {
return qualifier == null ? name() : qualifier + "." + name();
}

public String fullyQualifiedName() {
Copy link
Member

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.

return qualifier == null ? name() : buildRemoteIndexName(cluster, qualifier) + "." + name();
}

@Override
public Nullability nullable() {
return nullability;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,30 @@ public void testQueryCatalogPrecedence() throws Exception {
}
}

public void testQueryWithQualifierAndSetCatalog() throws Exception {
try (Connection es = esJdbc()) {
PreparedStatement ps = es.prepareStatement("SELECT " + INDEX_NAME + ".zero FROM " + INDEX_NAME);
es.setCatalog(REMOTE_CLUSTER_NAME);
ResultSet rs = ps.executeQuery();
assertTrue(rs.next());
assertEquals(0, rs.getInt(1));
assertFalse(rs.next());
}
}

public void testQueryWithQualifiedFieldAndIndex() throws Exception {
try (Connection es = esJdbc()) {
PreparedStatement ps = es.prepareStatement(
"SELECT " + INDEX_NAME + ".zero FROM " + buildRemoteIndexName(REMOTE_CLUSTER_NAME, INDEX_NAME)
);
es.setCatalog(LOCAL_CLUSTER_NAME); // set, but should be ignored
ResultSet rs = ps.executeQuery();
assertTrue(rs.next());
assertEquals(0, rs.getInt(1));
assertFalse(rs.next());
}
}

public void testCatalogDependentCommands() throws Exception {
for (String query : List.of(
"SHOW TABLES \"" + INDEX_NAME + "\"",
Expand Down