ESQL: Fixed length vector builder #99970
Conversation
This adds things like `IntVector.FixedBuilder` which is slightly simpler to use than constructing the arrays by hand. It also measures bytes used up front in the circuit breaker. And it'll be easier to integrate it into framework happening over in elastic#99931 to handle errors in topn. This also uses it in `mv_` functions.
elasticsearchmachine
added
the
Team:QL (Deprecated)
|
Pinging @elastic/es-ql (Team:QL) |
|
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
| return size == 1 | ||
| ? ConstantIntVector.RAM_BYTES_USED | ||
| : IntArrayVector.BASE_RAM_BYTES_USED + RamUsageEstimator.alignObjectSize( | ||
| (long) RamUsageEstimator.NUM_BYTES_ARRAY_HEADER + size * Integer.BYTES |
There was a problem hiding this comment.
This is fine. Separately we should update IntArrayVector::ramBytesEstimated, and have it based on an array length, rather than an array - then it can be used here as well as for the built vector. ( I think you made a similar comment on that original PR )
There was a problem hiding this comment.
I did! I see what you mean though, we can make these static methods on the classes. Will check.
| @@ -38,7 +38,7 @@ public String name() { | |||
| public Block evalNullable(Block fieldVal) { | |||
| DoubleBlock v = (DoubleBlock) fieldVal; | |||
| int positionCount = v.getPositionCount(); | |||
| DoubleBlock.Builder builder = DoubleBlock.newBlockBuilder(positionCount); | |||
| DoubleBlock.Builder builder = DoubleBlock.newBlockBuilder(positionCount, driverContext.blockFactory()); | |||
| /** | ||
| * A builder that never grows. | ||
| */ | ||
| sealed interface FixedBuilder extends Vector.Builder permits BooleanVectorFixedBuilder { |
There was a problem hiding this comment.
Sealed interfaces, nice!
| return this; | ||
| } | ||
|
|
||
| private static long size(int size) { |
There was a problem hiding this comment.
nit: got confused for a little here because of names. Maybe something like this would be clearer?
| private static long size(int size) { | |
| private static long size(int length) { |
(could also be applied to the constructor) or
private static long bytesUsed(int size)There was a problem hiding this comment.
Ah! I see. I think size is quite reasonable for the number of elements. But ramBytesUsed is actually the name we use for this in other places so I'll steal that.
| * The next byte to write into. {@code -1} means the vector has already | ||
| * been built. | ||
| */ | ||
| private int i; |
There was a problem hiding this comment.
nit:
| private int i; | |
| private int nextIndex; |
| private final BlockFactory blockFactory; | ||
| private final double[] values; | ||
| /** | ||
| * The next byte to write into. {@code -1} means the vector has already |
There was a problem hiding this comment.
Nit: this is not always a byte for all data types.
| * The next byte to write into. {@code -1} means the vector has already | |
| * The next position to write into. {@code -1} means the vector has already |
| // vectorBuilder(10, blockFactory).close(); | ||
| // assertThat(blockFactory.breaker().getUsed(), equalTo(0L)); |
There was a problem hiding this comment.
leftovers? Test doesn't seem to do anything.
There was a problem hiding this comment.
yup. letfover. When I port this over to my tracking branch that'll need something. I'll update.
nik9000
added
the
auto-merge-without-approval
|
@luigidellaquila would you be willing to drag this thing into more places? Like, maybe the conversion functions and evals? |
This adds things like `IntVector.FixedBuilder` which is slightly simpler to use than constructing the arrays by hand. It also measures bytes used up front in the circuit breaker. And it'll be easier to integrate it into framework happening over in elastic#99931 to handle errors in topn. This also uses it in `mv_` functions.
This adds things like
IntVector.FixedBuilderwhich is slightly simpler to use than constructing the arrays by hand. It also measures bytes used up front in the circuit breaker. And it'll be easier to integrate it into framework happening over in #99931 to handle errors in topn.This also uses it in
mv_functions.