-
Notifications
You must be signed in to change notification settings - Fork 25.3k
ESQL: Prevent search functions work with a non-STANDARD index #130638
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
Conversation
This introduces verifications to prevent search functions work on fields introduced by a LOOKUP JOIN righthand-side index.
Hi @bpintea, I've created a changelog YAML for you. |
Hi @bpintea, I've updated the changelog YAML for you. |
Pinging @elastic/es-analytical-engine (Team:Analytics) |
@bpintea search functions can be executed within the compute engine. Is there something specific we should do on the LOOKUP JOIN side for the engine to use them? |
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 from the prevention side - Would like to know more about how we can enable these functions in LOOKUP JOIN 🙂
@carlosdelest, it looks like it'll require dedicated work: from the quick look I had, the PS. Since (some of?) these can be evaluated internally, should we enable the |
Got it, thanks! 👍
We could. There are a number of restrictions that we're looking to lift.
That's trickier - full text functions still depend on Lucene, even when being evaluated on the compute engine, so they depend on the field being indexed. We could implement them on a computed field in case we created a Lucene in-memory index - something doable, but it hasn't been prioritized yet. |
Right, right. I think EVAL'ing might be handier, this the computed-value idea can wait (also for broader consensus for such a need). |
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.
Thanks a lot @bpintea ! Only minor comments from me, please consider them at your own discretion.
plan.forEachDown(p -> { | ||
if (p instanceof EsRelation esRelation && esRelation.indexMode() != IndexMode.STANDARD) { | ||
// Check if this EsRelation supplies the field | ||
if (esRelation.output().stream().anyMatch(attr -> attr.id().equals(fieldAttribute.id()))) { |
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.
Wouldn't it be more efficient to do esRelation.outputSet().contains(fieldAttribute)
?
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.
It would, ofc, thx!
}; | ||
} | ||
|
||
public static void fieldVerifier(LogicalPlan plan, FullTextFunction function, Supplier<Expression> fieldSupplier, Failures failures) { |
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.
nit: a more natural placement would maybe be inside FullTextFunction
rather than Match
, as other non-inheritors of Match
also reach for this method?
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.
Yes, was a lost intention, thanks.
}; | ||
} | ||
|
||
public static void fieldVerifier(LogicalPlan plan, FullTextFunction function, Supplier<Expression> fieldSupplier, Failures failures) { |
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.
question: why is fieldSupplier
a supplier, rather than using a simple Expression
? Seems like we hand it () -> field
every time.
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.
It no longer needs to, refactored.
Thanks Carlos, Alex! |
…c#130638) This introduces verifications to prevent search functions work on fields introduced by a LOOKUP JOIN righthand-side index. This should be a temporary fix until we can either push these filters down also on the righthand-side of a JOIN or have these functions execute within the engine. Closes elastic#130561 Closes elastic#129778
💔 Backport failed
You can use sqren/backport to manually backport by running |
…130638) (#130646) * ESQL: Prevent search functions work with a non-STANDARD index (#130638) This introduces verifications to prevent search functions work on fields introduced by a LOOKUP JOIN righthand-side index. This should be a temporary fix until we can either push these filters down also on the righthand-side of a JOIN or have these functions execute within the engine. Closes #130561 Closes #129778 * adapt test to 9.1
…130638) (#130704) * ESQL: Prevent search functions work with a non-STANDARD index (#130638) This introduces verifications to prevent search functions work on fields introduced by a LOOKUP JOIN righthand-side index. This should be a temporary fix until we can either push these filters down also on the righthand-side of a JOIN or have these functions execute within the engine. Closes #130561 Closes #129778 (cherry picked from commit 5c4d17d) * 8.19 test addaptation
This introduces verifications to prevent search functions work on fields introduced by a LOOKUP JOIN righthand-side index.
This should be a temporary fix until we can either push these filters down also on the righthand-side of a JOIN or have these functions execute within the engine.
Closes #130561
Closes #129778