Skip to content

Commit 1321c41

Browse files
jingwen-zjwjingwen-zjw
authored
Merge pull request #1 from jingwen-zjw/134468_throw_errors_on_potentially_ambiguous
134468 throw errors on potentially ambiguous
2 parents 71d9d17 + 1837d98 commit 1321c41

File tree

7 files changed

+272
-28
lines changed

7 files changed

+272
-28
lines changed

‎docs/changelog/136447.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 136447
2+
summary: "Add ambiguity check for unqualified attribute names in Analyzer"
3+
area: SQL
4+
type: enhancement
5+
issues: [134468]

‎server/src/main/java/org/elasticsearch/index/query/FieldNameValidator.java‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
package org.elasticsearch.index.query;
2+
3+
import org.elasticsearch.common.ParsingException;
4+
import org.elasticsearch.index.mapper.FieldMapper;
5+
import org.elasticsearch.index.mapper.Mapper;
6+
import org.elasticsearch.xcontent.XContentParser;
7+
8+
import java.util.ArrayList;
9+
import java.util.List;
10+
11+
/**
12+
* Utility class to validate field names for future shorthand support.
13+
*/
14+
public final class FieldNameValidator {
15+
16+
private FieldNameValidator() {}
17+
18+
/**
19+
* Ensures that the given field name is either fully qualified or
20+
* if it is a shorthand name, it uniquely resolves to a single field.
21+
*
22+
* @param fieldName the field name provided in the query DSL
23+
* @param context the SearchExecutionContext (used to inspect all mapped fields)
24+
*/
25+
public static void ensureShorthandSafe(String fieldName, SearchExecutionContext context) {
26+
if (fieldName.contains(".")) {
27+
return;
28+
}
29+
30+
List<String> matches = new ArrayList<>();
31+
32+
for (Mapper mapper : context.getMappingLookup().fieldMappers()) {
33+
String fullName = mapper.fullPath();
34+
if (fullName.equals(fieldName) || fullName.endsWith("." + fieldName)) {
35+
matches.add(fullName);
36+
}
37+
}
38+
39+
if (matches.size() > 1) {
40+
throw new IllegalArgumentException(
41+
"Ambiguous shorthand field name '" + fieldName + "'. " +
42+
"It matches multiple fields: " + matches + ". " +
43+
"Please use a fully qualified name like 'object." + fieldName + "'."
44+
);
45+
}
46+
}
47+
48+
public static void ensureSyntaxSafe(String fieldName, XContentParser parser) {
49+
if (fieldName == null || fieldName.isEmpty()) {
50+
throw new ParsingException(parser.getTokenLocation(), "Field name must not be empty");
51+
}
52+
}
53+
}

‎server/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -239,8 +239,15 @@ public static TermsQueryBuilder fromXContent(XContentParser parser) throws IOExc
239239
String queryName = null;
240240
float boost = AbstractQueryBuilder.DEFAULT_BOOST;
241241

242-
XContentParser.Token token;
243-
String currentFieldName = null;
242+
String currentFieldName = parser.currentName();
243+
fieldName = currentFieldName;
244+
FieldNameValidator.ensureSyntaxSafe(fieldName, parser);
245+
246+
XContentParser.Token token = parser.nextToken();;
247+
if (token != XContentParser.Token.FIELD_NAME) {
248+
throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] unknown token [" + token + "]");
249+
}
250+
244251
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
245252
if (token == XContentParser.Token.FIELD_NAME) {
246253
currentFieldName = parser.currentName();
@@ -322,6 +329,7 @@ public String getWriteableName() {
322329

323330
@Override
324331
protected Query doToQuery(SearchExecutionContext context) throws IOException {
332+
FieldNameValidator.ensureShorthandSafe(fieldName,context);
325333
if (termsLookup != null || supplier != null || values == null || values.isEmpty()) {
326334
throw new UnsupportedOperationException("query must be rewritten first");
327335
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1145,6 +1145,7 @@ private LogicalPlan resolveEval(Eval eval, List<Attribute> childOutput) {
11451145
* row foo = 1, bar = 2 | keep bar*, foo, * -> bar, foo
11461146
*/
11471147
private LogicalPlan resolveKeep(Project p, List<Attribute> childOutput) {
1148+
11481149
List<NamedExpression> resolvedProjections = new ArrayList<>();
11491150
var projections = p.projections();
11501151
// start with projections
@@ -1192,6 +1193,7 @@ private LogicalPlan resolveKeep(Project p, List<Attribute> childOutput) {
11921193
}
11931194

11941195
private LogicalPlan resolveDrop(Drop drop, List<Attribute> childOutput) {
1196+
11951197
List<NamedExpression> resolvedProjections = new ArrayList<>(childOutput);
11961198

11971199
for (var ne : drop.removals()) {
@@ -1343,15 +1345,39 @@ private DataType[] allowedEnrichTypes(String matchType) {
13431345
private static List<Attribute> resolveAgainstList(UnresolvedNamePattern up, Collection<Attribute> attrList) {
13441346
UnresolvedAttribute ua = new UnresolvedAttribute(up.source(), up.pattern());
13451347
Predicate<Attribute> matcher = a -> up.match(a.name());
1348+
1349+
// unified early ambiguity check
1350+
checkAmbiguousUnqualifiedName(ua, new ArrayList<>(attrList));
1351+
13461352
var matches = AnalyzerRules.maybeResolveAgainstList(matcher, () -> ua, attrList, true, a -> Analyzer.handleSpecialFields(ua, a));
13471353
return potentialCandidatesIfNoMatchesFound(ua, matches, attrList, list -> UnresolvedNamePattern.errorMessage(up.pattern(), list));
13481354
}
13491355

13501356
private static List<Attribute> resolveAgainstList(UnresolvedAttribute ua, Collection<Attribute> attrList) {
1357+
/// unified early ambiguity check
1358+
checkAmbiguousUnqualifiedName(ua, new ArrayList<>(attrList));
1359+
13511360
var matches = AnalyzerRules.maybeResolveAgainstList(ua, attrList, a -> Analyzer.handleSpecialFields(ua, a));
13521361
return potentialCandidatesIfNoMatchesFound(ua, matches, attrList, list -> UnresolvedAttribute.errorMessage(ua.name(), list));
13531362
}
13541363

1364+
private static void checkAmbiguousUnqualifiedName(UnresolvedAttribute ua, List<Attribute> available) {
1365+
if (ua.qualifier() != null) return; // qualified => not ambiguous here
1366+
1367+
final String name = ua.name();
1368+
List<Attribute> matches = available.stream()
1369+
.filter(a -> name.equals(a.name()))
1370+
.collect(java.util.stream.Collectors.toList());
1371+
1372+
if (matches.size() > 1) {
1373+
List<String> names = matches.stream()
1374+
.map(Attribute::name)
1375+
.collect(java.util.stream.Collectors.toList());
1376+
1377+
throw new VerificationException(UnresolvedAttribute.errorMessage(name, names));
1378+
}
1379+
}
1380+
13551381
private static List<Attribute> potentialCandidatesIfNoMatchesFound(
13561382
UnresolvedAttribute ua,
13571383
List<Attribute> matches,

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/AnalyzerRules.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,12 @@
77

88
package org.elasticsearch.xpack.esql.analysis;
99

10-
import org.elasticsearch.xpack.esql.VerificationException;
1110
import org.elasticsearch.xpack.esql.core.expression.Attribute;
1211
import org.elasticsearch.xpack.esql.core.expression.UnresolvedAttribute;
1312
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
1413
import org.elasticsearch.xpack.esql.rule.ParameterizedRule;
1514
import org.elasticsearch.xpack.esql.rule.Rule;
15+
import org.elasticsearch.xpack.esql.VerificationException;
1616

1717
import java.util.ArrayList;
1818
import java.util.Collection;
@@ -118,5 +118,6 @@ public static List<Attribute> maybeResolveAgainstList(
118118
return lineDiff != 0 ? lineDiff : (colDiff != 0 ? colDiff : a.name().compareTo(b.name()));
119119
}).map(a -> "line " + a.sourceLocation().toString().substring(1) + " [" + a.name() + "]").toList();
120120
throw new VerificationException("Found ambiguous reference to [" + ua.name() + "]; " + "matches any of " + refs);
121+
121122
}
122123
}

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerAmbiguityTests.java‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
package org.elasticsearch.xpack.esql.analysis;
9+
10+
import com.carrotsearch.randomizedtesting.RandomizedRunner;
11+
import org.elasticsearch.test.ESTestCase;
12+
import org.elasticsearch.xpack.esql.VerificationException;
13+
import org.elasticsearch.xpack.esql.core.expression.Attribute;
14+
import org.elasticsearch.xpack.esql.core.expression.UnresolvedAttribute;
15+
import org.junit.Test;
16+
import org.junit.runner.RunWith;
17+
18+
import java.lang.reflect.InvocationTargetException;
19+
import java.lang.reflect.Method;
20+
import java.util.Arrays;
21+
import java.util.List;
22+
import java.util.Locale;
23+
24+
import static org.mockito.Mockito.mock;
25+
import static org.mockito.Mockito.when;
26+
27+
@RunWith(RandomizedRunner.class)
28+
public class AnalyzerAmbiguityTests extends ESTestCase {
29+
30+
// Reflection: private static void checkAmbiguousUnqualifiedName(UnresolvedAttribute, List<Attribute>)
31+
private static Method checkMethod() throws NoSuchMethodException {
32+
Method m = Analyzer.class.getDeclaredMethod(
33+
"checkAmbiguousUnqualifiedName", UnresolvedAttribute.class, List.class
34+
);
35+
m.setAccessible(true);
36+
return m;
37+
}
38+
39+
private static UnresolvedAttribute ua(String name, String qualifierOrNull) {
40+
UnresolvedAttribute ua = mock(UnresolvedAttribute.class);
41+
when(ua.name()).thenReturn(name);
42+
when(ua.qualifier()).thenReturn(qualifierOrNull);
43+
return ua;
44+
}
45+
46+
private static Attribute attr(String name, String qualifierOrNull) {
47+
Attribute a = mock(Attribute.class);
48+
when(a.name()).thenReturn(name);
49+
when(a.qualifier()).thenReturn(qualifierOrNull);
50+
return a;
51+
}
52+
53+
// Accept both legacy "ambiguous reference" and newer "Unknown column ... did you mean ..." styles
54+
private static boolean isAmbiguityMessage(String msg, String name) {
55+
if (msg == null) return false;
56+
String lower = msg.toLowerCase(Locale.ROOT);
57+
return lower.contains("ambiguous") && lower.contains("[" + name.toLowerCase(Locale.ROOT) + "]");
58+
}
59+
60+
private static boolean isUnknownColumnSuggestion(String msg, String name) {
61+
if (msg == null) return false;
62+
// Match both “Unknown column[id]” and “Unknown column [id]”
63+
return msg.startsWith("Unknown column[" + name + "]")
64+
|| msg.startsWith("Unknown column [" + name + "]");
65+
}
66+
67+
@Test
68+
public void testAmbiguousUnqualifiedThrowsWhenQualifiersExist() throws Exception {
69+
// Two attributes with same name but different qualifiers => ambiguous for an unqualified reference
70+
var attrs = Arrays.asList(attr("id", "a"), attr("id", "b"), attr("name", "a"));
71+
var id = ua("id", null);
72+
try {
73+
checkMethod().invoke(null, id, attrs);
74+
fail("Expected VerificationException due to ambiguous unqualified name 'id'");
75+
} catch (InvocationTargetException ite) {
76+
Throwable cause = ite.getCause();
77+
assertTrue("Expected VerificationException, got: " + cause, cause instanceof VerificationException);
78+
String msg = cause.getMessage();
79+
// Accept both formats
80+
assertTrue("Unexpected message: " + msg,
81+
isAmbiguityMessage(msg, "id") || isUnknownColumnSuggestion(msg, "id"));
82+
}
83+
}
84+
85+
@Test
86+
public void testQualifiedNameBypassesAmbiguityCheck() throws Exception {
87+
// Qualified reference should not be considered ambiguous
88+
var attrs = Arrays.asList(attr("id", "a"), attr("id", "b"));
89+
var qualified = ua("id", "a");
90+
checkMethod().invoke(null, qualified, attrs); // Should not throw
91+
}
92+
93+
@Test
94+
public void testNoQualifiersButDuplicateNamesShouldThrow() throws Exception {
95+
// All qualifiers are null; duplicate simple names still make the reference ambiguous
96+
var attrs = Arrays.asList(
97+
attr("id", null),
98+
attr("id", null),
99+
attr("name", null)
100+
);
101+
var id = ua("id", null); // unqualified reference
102+
103+
// Call resolveAgainstList(UnresolvedAttribute, Collection) which now performs early ambiguity check
104+
Method m = Analyzer.class.getDeclaredMethod(
105+
"resolveAgainstList", UnresolvedAttribute.class, java.util.Collection.class
106+
);
107+
m.setAccessible(true);
108+
109+
try {
110+
m.invoke(null, id, attrs);
111+
fail("Expected VerificationException due to ambiguous unqualified name 'id' (even when qualifiers are null)");
112+
} catch (InvocationTargetException ite) {
113+
Throwable cause = ite.getCause();
114+
assertTrue("Expected VerificationException, got: " + cause, cause instanceof VerificationException);
115+
String msg = cause.getMessage();
116+
// Accept both formats
117+
assertTrue("Unexpected message: " + msg,
118+
isAmbiguityMessage(msg, "id") || isUnknownColumnSuggestion(msg, "id"));
119+
}
120+
}
121+
}

0 commit comments

Comments
 (0)