Skip to content

Commit b880642

Browse files
bratsethbratseth
authored
Merge pull request #36289 from vespa-engine/arnej/semantics-changes
Fix semantic rule production item insertion
2 parents e27988d + 034acd7 commit b880642

7 files changed

Lines changed: 53 additions & 30 deletions

File tree

‎container-search/src/main/java/com/yahoo/prelude/semantics/engine/Evaluation.java‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -100,9 +100,9 @@ public void reset() {
100100
/** Sets the item iterator to point to the last item: */
101101
public void setToLast() {
102102
if (flattenedItems != null)
103-
currentIndex = flattenedItems.size() - 1;
103+
currentIndex = flattenedItems.size() - 1;
104104
else
105-
currentIndex = -1;
105+
currentIndex = -1;
106106
}
107107

108108
/** Resets the item iterator to point to the last item: */
@@ -285,7 +285,8 @@ public void insertItems(List<Item> items, CompositeItem parent, int index, TermT
285285
}
286286

287287
if (( desiredParentType == TermType.DEFAULT || desiredParentType.hasItemClass(parent.getClass()) )
288-
&& equalIndexNameIfParentIsPhrase(items, parent)) {
288+
&& equalIndexNameIfParentIsPhrase(items, parent))
289+
{
289290
for (Item item : items)
290291
addItem(parent, index, item, desiredParentType);
291292
}
@@ -362,13 +363,12 @@ private void insertWithDesiredParentType(List<Item> items, int index, CompositeI
362363
return;
363364
}
364365

365-
for (Item item : items)
366-
newParent.addItem(item);
367-
368366
Item current = parent;
369367
if (parent instanceof QueryTree && parent.getItemCount() > 0)
370368
current = parent.getItem(0);
371369
if (current instanceof CompositeItem && !replacing) { // insert new parent below the current
370+
for (Item item : items)
371+
newParent.addItem(item);
372372
if (parent.getItemCount() > index) {
373373
var combinedItem = combineItems(newParent, parent.getItem(index), desiredParentType);
374374
parent.setItem(index, combinedItem);
@@ -379,6 +379,8 @@ private void insertWithDesiredParentType(List<Item> items, int index, CompositeI
379379
}
380380
else if (newParent.acceptsItemsOfType(current.getItemType())) { // insert new parent above the current
381381
newParent.addItem(current);
382+
for (Item item : items)
383+
newParent.addItem(item);
382384
if (newParent != parentsParent) { // Insert new parent as root or child of old parent's parent
383385
if (parentsParent != null)
384386
parentsParent.setItem(parentsParent.getItemIndex(current), newParent);
@@ -387,6 +389,8 @@ else if (newParent.acceptsItemsOfType(current.getItemType())) { // insert new pa
387389
}
388390
}
389391
else {
392+
for (Item item : items)
393+
newParent.addItem(item);
390394
((CompositeItem)current).addItem(newParent); // not an acceptable child -> composite
391395
}
392396
}
@@ -457,15 +461,11 @@ else if ((item instanceof CompositeItem) && ((CompositeItem)item).getItemCount()
457461
private CompositeItem createType(TermType termType) {
458462
if (termType == TermType.DEFAULT) {
459463
if (query.getModel().getType() == Query.Type.ANY)
460-
return new OrItem();
464+
termType = TermType.OR;
461465
else if (query.getModel().getType() == Query.Type.WEAKAND)
462-
return new WeakAndItem();
463-
else
464-
return new AndItem();
465-
}
466-
else {
467-
return (CompositeItem)termType.createItemClass();
466+
termType = TermType.WEAK_AND;
468467
}
468+
return (CompositeItem)termType.createItemClass();
469469
}
470470

471471
private void flatten(Item item,int position,List<FlattenedItem> toList) {

‎container-search/src/main/java/com/yahoo/prelude/semantics/rule/LiteralTermProduction.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,11 @@ else if (shouldInsertAtMatch(e)) {
8080
Match matched = e.getNonreferencedMatch(0);
8181
insertMatch(e, matched, List.of(newItem), offset);
8282
}
83+
else if (e.getNonreferencedMatchCount() > 0 && parentHasCompatibleType(e.getNonreferencedMatch(0))) {
84+
// Parent already has the right type (e.g., OR inside OR) - insert after match
85+
Match matched = e.getNonreferencedMatch(0);
86+
insertMatch(e, matched, List.of(newItem), offset + 1);
87+
}
8388
else {
8489
// Use root-level combining for specific types (RANK, OR, etc.) and non-nested cases
8590
e.addItems(List.of(newItem), getTermType());

‎container-search/src/main/java/com/yahoo/prelude/semantics/rule/ReferenceTermProduction.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,9 +130,13 @@ private void produce(RuleEvaluation e, Match match, List<Item> items, int offset
130130
insertMatch(e, match, items, offset);
131131
}
132132
else if (shouldInsertAtMatch(match)) {
133-
// Add to the match's parent when it's a nested composite (handles WeakAnd correctly)
133+
// Add to the match's parent when it's a nested composite with default type
134134
insertMatch(e, match, items, offset);
135135
}
136+
else if (parentHasCompatibleType(match)) {
137+
// Parent already has the right type - insert after match
138+
insertMatch(e, match, items, offset + 1);
139+
}
136140
else {
137141
// Use root-level combining for other cases
138142
e.addItems(items, getTermType());

‎container-search/src/main/java/com/yahoo/prelude/semantics/rule/TermProduction.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,4 +180,18 @@ protected boolean shouldInsertAtMatch(Match match) {
180180
return grandparent != null && !(grandparent instanceof QueryTree);
181181
}
182182

183+
/**
184+
* Returns true if the match's parent already has the same type as this production's term type,
185+
* and is nested (not directly under root). In this case we should add to the existing parent
186+
* rather than creating a new one at root level.
187+
*/
188+
protected boolean parentHasCompatibleType(Match match) {
189+
if (getTermType() == TermType.DEFAULT) return false;
190+
CompositeItem parent = match.getParent();
191+
if (parent == null) return false;
192+
if (!getTermType().hasItemClass(parent.getClass())) return false;
193+
CompositeItem grandparent = parent.getParent();
194+
return grandparent != null && !(grandparent instanceof QueryTree);
195+
}
196+
183197
}

‎container-search/src/test/java/com/yahoo/prelude/semantics/test/MatchOnlyIfNotOnlyTermTestCase.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,8 @@ public MatchOnlyIfNotOnlyTermTestCase() {
1717

1818
@Test
1919
void testMatch() {
20-
assertSemantics("RANK showname:\"saturday night live\"!1000 (AND justin timberlake)", "justin timberlake snl");
21-
assertSemantics("RANK showname:\"saturday night live\"!1000 (AND justin timberlake)", "justin timberlake saturday night live");
20+
assertSemantics("RANK (AND justin timberlake) showname:\"saturday night live\"!1000", "justin timberlake snl");
21+
assertSemantics("RANK (AND justin timberlake) showname:\"saturday night live\"!1000", "justin timberlake saturday night live");
2222
}
2323

2424
@Test

‎container-search/src/test/java/com/yahoo/prelude/semantics/test/OrPhraseTestCase.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,13 @@ public OrPhraseTestCase() {
1414

1515
@Test
1616
void testReplacing1() {
17-
assertSemantics("OR title:\"software engineer\" (AND new york)", "software engineer new york");
17+
assertSemantics("OR (AND new york) title:\"software engineer\"", "software engineer new york");
1818
assertSemantics("title:\"software engineer\"", "software engineer"); // Skip OR when there is nothing else
1919
}
2020

2121
@Test
2222
void testReplacing2() {
23-
assertSemantics("OR \"lord of the rings\" lotr", "lotr");
23+
assertSemantics("OR lotr \"lord of the rings\"", "lotr");
2424
}
2525

2626
@Test

‎container-search/src/test/java/com/yahoo/prelude/semantics/test/SemanticSearcherTestCase.java‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -131,12 +131,12 @@ void testMoreStopWordRemoval() {
131131

132132
@Test
133133
void testTypeChange() {
134-
assertSemantics("RANK default:typechange doors", "typechange doors");
134+
assertSemantics("RANK doors default:typechange", "typechange doors");
135135
}
136136

137137
@Test
138138
void testTypeChangeWithSingularToPluralButNonReplaceWillNotSingularify() {
139-
assertSemantics("RANK default:typechange door", "typechange door");
139+
assertSemantics("RANK door default:typechange", "typechange door");
140140
}
141141

142142
@Test
@@ -147,11 +147,12 @@ void testExplicitContext() {
147147
@Test
148148
void testOrProduction() {
149149
assertSemantics("OR something somethingelse", "something");
150+
150151
// I did not expect this:
151152
assertSemantics("OR (AND foo1 something bar2) somethingelse", "foo1 something bar2");
152-
// Nor this; we should fix documentation to emphasize that "+>" adding terms
153-
// always happens at the root of the query:
154-
assertSemantics("OR (RANK (AND foo1 (OR foo2 something bar1) bar2) bar3) somethingelse",
153+
154+
// but at least works like expected:
155+
assertSemantics("RANK (AND foo1 (OR foo2 something somethingelse bar1) bar2) bar3",
155156
"foo1 AND (foo2 OR something OR bar1) AND bar2 RANK bar3",
156157
0, Query.Type.ADVANCED);
157158
}
@@ -160,7 +161,7 @@ void testOrProduction() {
160161
void testDoubleOrProduction() {
161162
assertSemantics("OR more evenmore", "somethingmore");
162163
// Strange ordering:
163-
assertSemantics("OR more (AND foo1 bar2) evenmore", "foo1 somethingmore bar2");
164+
assertSemantics("OR (AND foo1 bar2) more evenmore", "foo1 somethingmore bar2");
164165
}
165166

166167
// This test is order dependent. Fix it!!
@@ -186,8 +187,8 @@ void testPhraseReplacementCornerCase() {
186187
assertSemantics("brand:smashtogether", "\"smash together\"");
187188
assertSemantics("brand:smashtogether", "smash-together");
188189
assertSemantics("AND foo1 brand:smashtogether bar2", "foo1 \"smash together\" bar2");
189-
assertSemantics("AND brand:smashtogether \"foo1 bar2\"", "\"foo1 smash together bar2\"");
190-
assertSemantics("OR brand:smashtogether \"foo1 bar2\"", "\"foo1 smash together bar2\"", 0, Query.Type.ANY);
190+
assertSemantics("AND \"foo1 bar2\" brand:smashtogether", "\"foo1 smash together bar2\"");
191+
assertSemantics("OR \"foo1 bar2\" brand:smashtogether", "\"foo1 smash together bar2\"", 0, Query.Type.ANY);
191192
// the difference in ordering here is because the parsed query already has a WEAKAND root (with 1 child):
192193
assertSemantics("WEAKAND \"foo1 bar2\" brand:smashtogether", "\"foo1 smash together bar2\"", 0, Query.Type.WEAKAND);
193194
}
@@ -205,11 +206,10 @@ void testWandAddition() {
205206
@Test
206207
void testWandReplacement() {
207208
assertSemantics("WEAKAND greatest", "goat");
208-
assertSemantics("WEAKAND greatest dazed", "dazed goat");
209-
assertSemantics("WEAKAND greatest (AND dazed disoriented)", "dazed goat disoriented");
209+
assertSemantics("WEAKAND dazed greatest", "dazed goat");
210+
assertSemantics("WEAKAND (AND dazed disoriented) greatest", "dazed goat disoriented");
210211
assertSemantics("WEAKAND the greatest of all time", "thegoat");
211-
// Strange ordering again:
212-
assertSemantics("WEAKAND the (AND dazed disoriented) greatest of all time", "dazed thegoat disoriented");
212+
assertSemantics("WEAKAND (AND dazed disoriented) the greatest of all time", "dazed thegoat disoriented");
213213
}
214214

215215
private Result doSearch(Searcher searcher, Query query, int offset, int hits) {

0 commit comments

Comments
 (0)
⚡