ESQL: Read from the `BlockFactory` #100231

nik9000 · 2023-10-03T20:37:33Z

This links the BlockFactory into the Block serialization code. With this blocks that are deserialized from over the wire are tracked.

This links the `BlockFactory` into the `Block` serialization code. With this blocks that are deserialized from over the wire are tracked.

elasticsearchmachine · 2023-10-03T20:37:58Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2023-10-03T20:37:59Z

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

nik9000 · 2023-10-03T20:41:17Z

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/ConstantNullBlock.java

@@ -68,6 +68,7 @@ public ElementType elementType() {

    @Override
    public Block filter(int... positions) {
+        close();


This is so weird. Usually filter preserves a reference to the underlying block. But it doesn't for ConstantNullBlock and that's fine and good. But we presume that it does. This "simulates" that. I hate it.

Yeah. this finds sucks. It's ok for now.

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/Page.java

nik9000 · 2023-10-03T20:44:05Z

...ugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/EsqlActionIT.java

@@ -861,6 +861,7 @@ public void testFromStatsLimit() {
        }
    }

+    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/99826")
    public void testFromLimit() {
        try (EsqlQueryResponse results = run("from test | keep data | limit 2")) {


There's something funky going on with LimitOperator and I tried to track it down - you can see the tests I added. But I ultimately gave up. I think the problem here is that we finish the Operator and something on the exchange doesn't close. I'm not sure though.

nik9000 · 2023-10-03T20:44:55Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/lookup/EnrichLookupIT.java

@@ -52,6 +53,7 @@

 import static org.hamcrest.Matchers.equalTo;

+@LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/99826")


I hate disabling this. ENRICH seems to work, but the IT isn't. It could be that we simply don't test the enrich path I'm breaking with this PR though.

You can leave it muted. I will integrate BlockFactory to the enrich operator.

thanks. I was digging around and learning a lot. But it wasn't quick.

dnhatn

This looks good. I've left a comment regarding the passing of the BlockFactory.

dnhatn · 2023-10-04T06:12:08Z

.../plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/TransportEsqlQueryAction.java

+         * to BigArrays, which we need to build the BlockFactory
+         * up front.
+         */
+        blockFactoryHolder.blockFactory = blockFactory;


Can we create a custom StreamInput, such as PlanStreamInput, and then create and pass a BlockFactory whenever we deserialize a Page? Currently, we only deserialize Pages in two places: ExchangeResponse and LookupResponse for enrich.

I'll have a look. I figured it'd amount to the same thing though because we have to figure out how to do that passing. But maybe it's later. I'll look.

I did it! I do like this better.

ChrisHegarty

This is mostly very good. I think there is just a couple of outstanding comments.

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/Page.java

ChrisHegarty · 2023-10-04T08:45:46Z

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/LimitOperator.java

-
+        if (lastInput != null) {
+            lastInput.releaseBlocks();
+        }


...lugin/esql/compute/src/test/java/org/elasticsearch/compute/data/BlockSerializationTests.java

ChrisHegarty · 2023-10-04T08:52:35Z

...plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/plugin/CanMatchIT.java

-            assertThat(getValuesList(resp), hasSize(0));
-            assertThat(queriedIndices, empty());
-            queriedIndices.clear();
+            try (EsqlQueryResponse resp = run("from events_*", randomPragmas(), new RangeQueryBuilder("@timestamp").gte("2023-01-01"))) {


Hmm.. I updated these tests to close the response, so created a merge conflict here. Sorry.

Merge conflicts are life.

ChrisHegarty · 2023-10-04T08:53:24Z

.../plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/TransportEsqlQueryAction.java

+         * to BigArrays, which we need to build the BlockFactory
+         * up front.
+         */
+        blockFactoryHolder.blockFactory = blockFactory;


nik9000 · 2023-10-04T19:34:02Z

@dnhatn and @ChrisHegarty, could you have another look? I've moved the BlockFactory to a StreamInput subclass. I intentionally only plugged it into the ExchangeResponse for now. Because ENRICH doesn't use the factory at all yet I figured that was safer. And we could plug that in later.

nik9000 · 2023-10-04T20:17:40Z

The part 2 failure looks real. I'll look!

dnhatn · 2023-10-04T23:01:07Z

The part 2 failure looks real. I'll look!

@nik9000 #100259 helps releasing discarded Pages in ExchangeBuffer. And I pushed 63e6473 to fix the tests where manipulated ExchangeResponse. We should be good here.

dnhatn

I left a comment, but LGTM.

dnhatn · 2023-10-04T22:40:36Z

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/X-Block.java.st

-                builder.beginPositionEntry();
-                for (int valueIndex = 0; valueIndex < valueCount; valueIndex++) {
-                    builder.append$Type$(in.read$Type$());
+        try ($Type$Block.Builder builder = ((BlockStreamInput) in).blockFactory().new$Type$BlockBuilder(positions)) {


I think we prefer not to have a type cast here, but it should not block this PR.

I'll grab that in a followup.

This links the `BlockFactory` into the `Block` serialization code. With this blocks that are deserialized from over the wire are tracked. Co-authored-by: Nhat Nguyen <[email protected]>

elasticsearchmachine · 2023-10-05T02:08:34Z

💚 Backport successful

Status	Branch	Result
✅	8.11

This links the `BlockFactory` into the `Block` serialization code. With this blocks that are deserialized from over the wire are tracked. Co-authored-by: Nhat Nguyen <[email protected]>

ESQL: Read from the BlockFactory

37a8fc9

This links the `BlockFactory` into the `Block` serialization code. With this blocks that are deserialized from over the wire are tracked.

nik9000 added >non-issue :Analytics/ES|QL AKA ESQL v8.11.0 labels Oct 3, 2023

nik9000 requested review from dnhatn and ChrisHegarty October 3, 2023 20:37

elasticsearchmachine added the Team:QL (Deprecated) Meta label for query languages team label Oct 3, 2023

nik9000 commented Oct 3, 2023

View reviewed changes

Merge branch 'main' into read_from_block_factory

cd7876f

nik9000 mentioned this pull request Oct 3, 2023

Block and Vector creation can start to use a block factory that performs circuit breaking #99826

Closed

15 tasks

dnhatn reviewed Oct 4, 2023

View reviewed changes

ChrisHegarty reviewed Oct 4, 2023

View reviewed changes

mattc58 added v8.12.0 and removed v8.11.0 labels Oct 4, 2023

nik9000 added auto-backport Automatically create backport pull requests when merged v8.11.0 labels Oct 4, 2023

nik9000 added 6 commits October 4, 2023 09:52

Merge branch 'main' into read_from_block_factory

724788c

Merge update

d3506ff

Merge branch 'main' into read_from_block_factory

e4811af

Oh

e9d6628

Spotless

f7fce9d

Move BlockFactory

326b7eb

dnhatn added 2 commits October 4, 2023 15:56

Merge remote-tracking branch 'elastic/main' into read_from_block_factory

c91e1f2

Fix tests

63e6473

dnhatn approved these changes Oct 4, 2023

View reviewed changes

nik9000 added the auto-backport-and-merge label Oct 5, 2023

nik9000 merged commit 9f72ce0 into elastic:main Oct 5, 2023

nik9000 mentioned this pull request Oct 5, 2023

[8.11] ESQL: Read from the BlockFactory (#100231) #100310

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Read from the `BlockFactory` #100231

ESQL: Read from the `BlockFactory` #100231

nik9000 commented Oct 3, 2023

elasticsearchmachine commented Oct 3, 2023

elasticsearchmachine commented Oct 3, 2023

nik9000 Oct 3, 2023

ChrisHegarty Oct 4, 2023

nik9000 Oct 3, 2023

nik9000 Oct 3, 2023

dnhatn Oct 3, 2023

nik9000 Oct 3, 2023

dnhatn left a comment

dnhatn Oct 4, 2023

ChrisHegarty Oct 4, 2023

nik9000 Oct 4, 2023

nik9000 Oct 4, 2023

ChrisHegarty left a comment

ChrisHegarty Oct 4, 2023

ChrisHegarty Oct 4, 2023

nik9000 Oct 4, 2023

ChrisHegarty Oct 4, 2023

nik9000 commented Oct 4, 2023

nik9000 commented Oct 4, 2023

dnhatn commented Oct 4, 2023

dnhatn left a comment

dnhatn Oct 4, 2023

nik9000 Oct 5, 2023

elasticsearchmachine commented Oct 5, 2023

		@@ -52,6 +53,7 @@

		import static org.hamcrest.Matchers.equalTo;

		@LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/99826")

ESQL: Read from the BlockFactory #100231

ESQL: Read from the BlockFactory #100231

Conversation

nik9000 commented Oct 3, 2023

elasticsearchmachine commented Oct 3, 2023

elasticsearchmachine commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnhatn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisHegarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Oct 4, 2023

nik9000 commented Oct 4, 2023

dnhatn commented Oct 4, 2023

dnhatn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Oct 5, 2023

💚 Backport successful

ESQL: Read from the `BlockFactory` #100231

ESQL: Read from the `BlockFactory` #100231