ExpressionResolver uses sets instead of lists #52788
Conversation
This commit changes the IndexNameExpressionResolver and the inner ExpressionResolver objects to use Sets instead of Lists in their APIs, which addresses a TODO left in this code.
jaymode
added
>non-issue
:Core/Infra/Core
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
It looks like this PR uses LinkedHashSet in place of all the previous Lists. Do we rely on the original ordering? If so, why are we converting to Set?
Yes we do because of the negation syntax,
Internally we convert to a set and then convert back to a List in the WildcardExpressionResolver. I was trying to address this TODO but I'm not sure this is the right change to make. @jpountz you left the original TODO, can you elaborate on the TODO and if you still think we should do this? |
|
I had not looked deeply into the consequences of moving to sets, but this code keeps going back and forth between lists and sets, which fels wasteful. |
|
I originally wrote this comment because expressions are initially provided as a list, and are then converted to a set in
This is a good point. It probably means that even the LinkedHashSet approach is not good, as providing a list of expressions that looks like
Yes, sorry for the time you spent on it, it looks like this TODO is buggy and we should remove it. |
Excellent point.
No worries. I just wanted the API to be more stable if we make the wildcard resolver pluggable. I will close this and open a PR removing the TODO |
This commit removes a TODO in the IndexNameExpressionResolver that indicated the API should use a Set instead of a List. However, this TODO was not completely correct since the ordering of arguments matters due to negations when evaluating wildcards and since we also allow a list of patterns like `*,-foo,*`, which would have a different meaning even when using a Set with insertion ordering. Relates elastic#52788
This commit removes a TODO in the IndexNameExpressionResolver that indicated the API should use a Set instead of a List. However, this TODO was not completely correct since the ordering of arguments matters due to negations when evaluating wildcards and since we also allow a list of patterns like `*,-foo,*`, which would have a different meaning even when using a Set with insertion ordering. Relates #52788
This commit removes a TODO in the IndexNameExpressionResolver that indicated the API should use a Set instead of a List. However, this TODO was not completely correct since the ordering of arguments matters due to negations when evaluating wildcards and since we also allow a list of patterns like `*,-foo,*`, which would have a different meaning even when using a Set with insertion ordering. Relates elastic#52788
This commit removes a TODO in the IndexNameExpressionResolver that indicated the API should use a Set instead of a List. However, this TODO was not completely correct since the ordering of arguments matters due to negations when evaluating wildcards and since we also allow a list of patterns like `*,-foo,*`, which would have a different meaning even when using a Set with insertion ordering. Relates #52788 Backport of #52963
3 participants
This commit changes the IndexNameExpressionResolver and the inner
ExpressionResolver objects to use Sets instead of Lists in their APIs,
which addresses a TODO left in this code.
Relates #40263