Copilot commented on code in PR #61548:
URL: https://github.com/apache/doris/pull/61548#discussion_r2963880085
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/SearchDslParser.java:
##########
@@ -277,6 +277,37 @@ private static void validateFieldsList(List<String>
fields) {
}
}
+ private static String buildFieldPath(SearchParser.FieldPathContext ctx) {
+ if (ctx == null) {
+ throw new RuntimeException("Invalid field query: missing field
path");
Review Comment:
buildFieldPath() throws a generic RuntimeException for a missing field path.
That ends up being wrapped as an "Unexpected error parsing search DSL" in
parseDsl*, which is misleading for user-input syntax issues. Prefer throwing
SearchDslSyntaxException here so callers consistently surface an "Invalid
search DSL" / syntax error message instead of an internal/unexpected error.
```suggestion
throw new SearchDslSyntaxException("Invalid field query: missing
field path");
```
##########
fe/fe-core/src/test/java/org/apache/doris/analysis/SearchPredicateTest.java:
##########
@@ -153,6 +153,35 @@ public void testBuildThriftParam() {
Assertions.assertEquals("content",
param.field_bindings.get(1).field_name);
}
+ @Test
+ public void testNestedRelativeFieldsAreNormalizedBeforeThrift() {
+ String dsl = "NESTED(data.items, msg:hello AND meta.channel:action)";
+ SearchDslParser.QsPlan plan = SearchDslParser.parseDsl(dsl,
"{\"mode\":\"standard\"}");
+ List<Expr> children = Arrays.asList(createTestSlotRef("data"),
createTestSlotRef("data"));
+
+ SearchPredicate predicate = new SearchPredicate(dsl, plan, children,
true);
+
+ TExprNode thriftNode = new TExprNode();
+ predicate.accept(ExprToThriftVisitor.INSTANCE, thriftNode);
+
+ TSearchParam param = thriftNode.search_param;
+ Assertions.assertNotNull(param);
+ Assertions.assertEquals("NESTED", param.root.clause_type);
+ Assertions.assertEquals("data.items", param.root.nested_path);
+ Assertions.assertEquals(1, param.root.children.size());
+ Assertions.assertEquals("AND", param.root.children.get(0).clause_type);
+ Assertions.assertEquals("data.items.msg",
param.root.children.get(0).children.get(0).field_name);
+ Assertions.assertEquals("data.items.meta.channel",
param.root.children.get(0).children.get(1).field_name);
+
+ Assertions.assertEquals(2, param.field_bindings.size());
+ Assertions.assertEquals("data.items.msg",
param.field_bindings.get(0).field_name);
+ Assertions.assertEquals("data",
param.field_bindings.get(0).parent_field_name);
+ Assertions.assertEquals("items.msg",
param.field_bindings.get(0).subcolumn_path);
+ Assertions.assertEquals("data.items.meta.channel",
param.field_bindings.get(1).field_name);
+ Assertions.assertEquals("data",
param.field_bindings.get(1).parent_field_name);
+ Assertions.assertEquals("items.meta.channel",
param.field_bindings.get(1).subcolumn_path);
Review Comment:
This test asserts specific ordering of param.field_bindings (indexes 0/1).
Field bindings are derived from AST traversal/set insertion order, so the exact
order can change with harmless refactors and make the test flaky. Consider
asserting the bindings by field_name (e.g., lookup by name or assert the set of
names/parent_field_name/subcolumn_path) rather than relying on list position.
```suggestion
Map<String, TSearchFieldBinding> bindingsByFieldName = new
HashMap<>();
for (TSearchFieldBinding binding : param.field_bindings) {
bindingsByFieldName.put(binding.field_name, binding);
}
TSearchFieldBinding msgBinding =
bindingsByFieldName.get("data.items.msg");
Assertions.assertNotNull(msgBinding);
Assertions.assertEquals("data", msgBinding.parent_field_name);
Assertions.assertEquals("items.msg", msgBinding.subcolumn_path);
TSearchFieldBinding metaChannelBinding =
bindingsByFieldName.get("data.items.meta.channel");
Assertions.assertNotNull(metaChannelBinding);
Assertions.assertEquals("data",
metaChannelBinding.parent_field_name);
Assertions.assertEquals("items.meta.channel",
metaChannelBinding.subcolumn_path);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
