-
Notifications
You must be signed in to change notification settings - Fork 25.7k
MV_CONTAINS avoid autoboxing #135991
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
MV_CONTAINS avoid autoboxing #135991
Conversation
…pe-specific blocks.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/X-Block.java.st
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/X-Block.java.st
Outdated
Show resolved
Hide resolved
|
@ivancea what's the blessed way to address the mixed-cluster tests failing? |
|
@mjmbischoff I see the error is the change of empty string behaviour. We'll need to make a new capability for it , to avoid running those tests against versions with the old behaviour. (Or rename the existing cap to something like |
.../plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanBlock.java
Outdated
Show resolved
Hide resolved
.../plugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanBlock.java
Outdated
Show resolved
Hide resolved
| final var count = getValueCount(valueIndex); | ||
| final var startIndex = getFirstValueIndex(valueIndex); | ||
| $if(BytesRef)$ | ||
| var ref = new BytesRef(); |
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.
I'd pass the scratch in for this in BytesRefBlock. One scratch per position feels like a lot.
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.
ah yes, long day. Renamed it to position.
I've moved the scratch up one level though in the back of my mind I do wonder if we'll have a net gain from this as we might just break escape analysis 'a stack too far' so to speak.
...main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvContains.java
Show resolved
Hide resolved
💔 Backport failedThe backport operation could not be completed due to the following error: You can use sqren/backport to manually backport by running |
Updated logic to avoid autoboxing by adding
hasValuemethods for type-specific blocks.Item on the road to GA of MV_CONTAINS, see discussion in #133636 and #133099. Other item is #135723