Conversation
optional fields as joining fields
|
Pinging @elastic/es-ql (Team:QL) |
costin
left a comment
costin
left a comment
There was a problem hiding this comment.
Besides the various code comments, I'd like to see more tests for fields that have ? in their name and combine that with optional fields:
by `?somefield`
by ?`?somefield`
where `some?field` == null
where ?`some?field` == null
where `somefield?`
where ?`somefield?`
[query] by `?somefield` // should work
[query] by ?`?somefield` // should throw parsing error
| } | ||
| } | ||
| }, | ||
| "no_docs_field": { |
There was a problem hiding this comment.
Improved name would be - "optional_field_mapping_only". Potentially drop optional.
| "no_docs_field": { | ||
| "type": "keyword" | ||
| }, | ||
| "default_null_value_field": { |
| Expression named = resolveAgainstList(u, childrenOutput); | ||
| // if it's not resolved (it doesn't exist in mappings) and it's an optional field, replace it with "null" | ||
| if (named == null && optionals.contains(u)) { | ||
| named = Literal.NULL; |
There was a problem hiding this comment.
you could preserve the source (for better error messaging):
new Literal(named.source(), null, DataTypes.NULL)
|
|
||
| public class AstBuilder extends LogicalPlanBuilder { | ||
|
|
||
| AstBuilder(ParserParams params) { |
There was a problem hiding this comment.
You could keep the old constructor around and make it a shortcut to this(params, emptySet())
| return ctx != null ? visitList(this, ctx.expression(), Attribute.class) : emptyList(); | ||
| List<ExpressionContext> keys = ctx.expression(); | ||
| for (ExpressionContext k : keys) { | ||
| if ("?".equals(k.getStart().getText())) { |
There was a problem hiding this comment.
I don't think this is correct - the check needs to happen on OPTIONAL not the ? string since it might get confused by a quoted identifier starting with ?, e.g.:
[queryA] by `?specialField`
| return new UnresolvedAttribute(source(ctx), visitQualifiedName(ctx.qualifiedName())); | ||
| String name = visitQualifiedName(ctx.qualifiedName()); | ||
| UnresolvedAttribute attr = new UnresolvedAttribute(source(ctx), name); | ||
| if ("?".equals(ctx.getStart().getText())) { |
There was a problem hiding this comment.
Same as above - the context is too low-level.
| private final Planner planner; | ||
| private final CircuitBreaker circuitBreaker; | ||
|
|
||
| private final Set<UnresolvedAttribute> optionals = new HashSet<>(); |
There was a problem hiding this comment.
Instead of passing this per session, it should be read from the parser.
There was a problem hiding this comment.
@costin Actually, this is the place where the list of optionals is "initialized" and then passed on to all necessary instances so that, in the end, gets populated in the expression builder. Later on, in the Analyzer, this list is checked in ResolveRefs.
| private final boolean DEBUG = false; | ||
| private final Set<UnresolvedAttribute> optionals; | ||
|
|
||
| public EqlParser(Set<UnresolvedAttribute> optionals) { |
There was a problem hiding this comment.
The set shouldn't be set to the constructor but rather the parser detect all the optional fields and return them accordingly. As it stands the caller is forced to pass an empty set for the parser to initialize it.
Luegg
left a comment
Luegg
left a comment
There was a problem hiding this comment.
This approach of populating a set of optional attributes is somewhat surprising to me. Why is the optionality not a property of UnresolvedAttribute?
Also, it looks to me that declaring an attribute as optional in one place makes the attribute optional everywhere, e.g. in any where ?foo == 1 or foo == 2. Is that the intended behavior? I could imagine that users might expect an error in this case (or at least if the foo attribute does not exist).
|
Superseded by #79677 |
5 participants
Add optional fields, with usage limited to queries only, excluding optional fields as joining fields.
Optional fields have a question mark in front of their name (
?some_field) and can be used in standalone event queries and sequences, but not as join fields.If the field exists in at least one index that's getting queried, that field will actually be used in queries.
If the field doesn't exist in any of the indices, all its mentions in query will be replaced with
null.