ESQL: Page shouldn't close a block twice #100370

costin · 2023-10-05T21:04:29Z

Page now takes into account that a block can be used in multiple
positions (such as the same column aliased under multiple names).

Relates #100001
Fix #100365
Fix #100356

Page now takes into account that a block can be used in multiple positions (such as the same column aliased under multiple names). Relates elastic#100001 Fix elastic#100365 Fix elastic#100356

elasticsearchmachine · 2023-10-05T21:04:55Z

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

elasticsearchmachine · 2023-10-05T21:04:55Z

Hi @costin, I've created a changelog YAML for you.

elasticsearchmachine · 2023-10-05T21:04:55Z

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

nik9000

@alex-spies should probably look at this too. It does get the job done. And we don't need to keep it forever. If we have reference counting we wouldn't need it. But we don't have it now.

nik9000 · 2023-10-05T21:25:09Z

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

@@ -222,6 +223,13 @@ public void writeTo(StreamOutput out) throws IOException {
     */
    public void releaseBlocks() {
        blocksReleased = true;
-        Releasables.closeExpectNoException(blocks);
+        // blocks can be used as multiple columns
+        var set = new IdentityHashMap<Block, Object>();


Collections.newSetFromMap is the idiomatic way to do this. And all the cool kids want to be idiots, right?

:)
I actually tried that first but it blew up because the blocks equals implementation checks the content meaning different blocks end up looking equal meaning they are not closed as they are considered a duplicate.
Which causes a bunch of leaks - hence why I'm using the Identity match alone.

nik9000 · 2023-10-05T21:26:07Z

@ChrisHegarty also might have opinions on this.

costin · 2023-10-05T23:36:22Z

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

+     * That is, allows clean-up of the current page _after_ external manipulation of the blocks.
+     * The current page should no longer be used and be considered closed.
+     */
+    public Page newPageAndRelease(Block... keep) {


I've introduced this method for handling the case of reusing some blocks from an old page into a new one.
This happens in ProjectExec and OutputExec (inside the planner) - I've moved this method from the Project into the page and clean-it up a bit (the usual reduction complexity from using two lists - O(N*M) to O(N) with the extra memory for the map).

costin · 2023-10-05T23:37:21Z

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

-        for (int i = 0; i < toAdd.length; i++) {
-            this.blocks[prev.blocks.length + i] = toAdd[i];
-        }
+        System.arraycopy(toAdd, 0, this.blocks, prev.blocks.length + 0, toAdd.length);


Small improvement .

are not-used when creating a new page

costin · 2023-10-05T23:38:00Z

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

-        for (int i = 0; i < toAdd.length; i++) {
-            this.blocks[prev.blocks.length + i] = toAdd[i];
-        }
+        System.arraycopy(toAdd, 0, this.blocks, prev.blocks.length, toAdd.length);


Small improvement.

costin · 2023-10-06T02:38:54Z

@elasticsearchmachine run elasticsearch-ci/part-1

costin · 2023-10-06T03:37:13Z

FTR, it looks like all the builders for part-1 end up being disabled...

alex-spies

LGTM as an intermediate fix.

IMO we're beginning to create a lot of workarounds to make up for a lack of ref counted shallow copies of blocks and that makes the code harder to reason about.

alex-spies · 2023-10-06T07:49:30Z

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

+        var map = new IdentityHashMap<Block, Object>(mapSize(blocks.length));
+        var DUMMY = new Object();
+        for (Block b : blocks) {
+            if (map.putIfAbsent(b, DUMMY) == null) {
+                Releasables.closeExpectNoException(b);
+            }
+        }


I think this improves the issue, but there still can be problems because non-identical blocks can be backed by the same vector or array.

Additionally, we create and populate a new hash map on each page release; that's probably quite expensive.

We can merge this to fix the problem right now, but IMO should mark this with a todo comment to remove this logic once possible. We want ref counting, this will fix the problem more idiomatically.

non-identical blocks can be backed by the same vector or array.

Why would they?

we create and populate a new hash map on each page release; that's probably quite expensive.

I've added a check to skip the release once it's one - we can change that if we want a page to only be released once.

alex-spies · 2023-10-06T07:51:23Z

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

-        Releasables.closeExpectNoException(blocks);
+        // blocks can be used as multiple columns
+        var map = new IdentityHashMap<Block, Object>(mapSize(blocks.length));
+        var DUMMY = new Object();


That could be moved into a static final var.

Or...

var map = new IdentityHashMap<Block, Boolean>(mapSize(blocks.length)); for (Block b : blocks) { if (map.putIfAbsent(b, Boolean.TRUE) == null) { ..

ChrisHegarty

LGTM. I left a few small comments / suggestions.

Additionally, it would be good to have a small unit test in BasicPageTests for the Page additions.

ChrisHegarty · 2023-10-06T09:24:59Z

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

-        Releasables.closeExpectNoException(blocks);
+        // blocks can be used as multiple columns
+        var map = new IdentityHashMap<Block, Object>(mapSize(blocks.length));
+        var DUMMY = new Object();


Or...

var map = new IdentityHashMap<Block, Boolean>(mapSize(blocks.length)); for (Block b : blocks) { if (map.putIfAbsent(b, Boolean.TRUE) == null) { ..

ChrisHegarty · 2023-10-06T09:42:21Z

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

+
+        // create identity set
+        for (Block b : keep) {
+            map.putIfAbsent(b, DUMMY);


Subjective:

// create identity set var set = Collections.newSetFromMap(new IdentityHashMap<Block, Boolean>(mapSize(keep.length))); set.addAll(Arrays.asList(keep)); .. if (set.contains(b) == false) { ..

bpintea

LGTM

bpintea · 2023-10-06T12:43:12Z

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

+    }
+
+    static int mapSize(int expectedSize) {
+        return expectedSize < 2 ? expectedSize + 1 : (int) (expectedSize / 0.75 + 1.0);


~~CollectionUtils.mapSize()?~~
Edit: not avail in compute package, disregard.

elasticsearchmachine · 2023-10-06T18:51:56Z

💔 Backport failed

Status	Branch	Result
❌	8.11	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 100370

Page now takes into account that a block can be used in multiple positions (such as the same column aliased under multiple names). Introduce newPageAndRelease method that handles clean-up of blocks that are not-used when creating a new page Relates elastic#100001 Fix elastic#100365 Fix elastic#100356 (cherry picked from commit 44068cb)

* ESQL: Page shouldn't close a block twice (#100370) Page now takes into account that a block can be used in multiple positions (such as the same column aliased under multiple names). Introduce newPageAndRelease method that handles clean-up of blocks that are not-used when creating a new page Relates #100001 Fix #100365 Fix #100356 (cherry picked from commit 44068cb) * Fix order inside test

ESQL: Page shouldn't close a block twice

a7cb6ab

Page now takes into account that a block can be used in multiple positions (such as the same column aliased under multiple names). Relates elastic#100001 Fix elastic#100365 Fix elastic#100356

costin added >bug :Analytics/ES|QL AKA ESQL v8.12.0 labels Oct 5, 2023

costin requested review from nik9000, dnhatn and ChrisHegarty October 5, 2023 21:04

costin self-assigned this Oct 5, 2023

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

Update docs/changelog/100370.yaml

63de76d

nik9000 reviewed Oct 5, 2023

View reviewed changes

costin commented Oct 5, 2023

View reviewed changes

Introduce newPageAndRelease method that handles clean-up of blocks that

7932c05

are not-used when creating a new page

costin force-pushed the fix/100001 branch from 7a0f1af to 7932c05 Compare October 5, 2023 23:37

costin commented Oct 5, 2023

View reviewed changes

costin added 2 commits October 5, 2023 17:20

Update project test

5cc7539

Merge branch 'main' into fix/100001

86ef454

costin mentioned this pull request Oct 6, 2023

ESQL: Ensure pages contain no duplicate blocks #100061

Closed

alex-spies approved these changes Oct 6, 2023

View reviewed changes

ChrisHegarty approved these changes Oct 6, 2023

View reviewed changes

ChrisHegarty mentioned this pull request Oct 6, 2023

Remove a couple of awaitsfix in EsqlActionIT #100391

Closed

bpintea approved these changes Oct 6, 2023

View reviewed changes

Address feedback

e3415a2

costin added buildkite-opt-in Opts your PR into Buildkite instead of Jenkins v8.11.1 auto-backport-and-merge labels Oct 6, 2023

costin merged commit 44068cb into elastic:main Oct 6, 2023

costin deleted the fix/100001 branch October 6, 2023 18:50

elasticsearchmachine added the backport pending label Oct 6, 2023

costin mentioned this pull request Oct 6, 2023

[8.11] ESQL: Page shouldn't close a block twice (#100370) #100457

Merged

This was referenced Oct 9, 2023

ProjectTests fails on some seed #99982

Closed

ESQL: Pages and blocks being closed #100001

Closed

bpintea removed the backport pending label Nov 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Page shouldn't close a block twice #100370

ESQL: Page shouldn't close a block twice #100370

costin commented Oct 5, 2023

elasticsearchmachine commented Oct 5, 2023

elasticsearchmachine commented Oct 5, 2023

elasticsearchmachine commented Oct 5, 2023

nik9000 left a comment

nik9000 Oct 5, 2023

costin Oct 5, 2023

nik9000 commented Oct 5, 2023

costin Oct 5, 2023

costin Oct 5, 2023

costin Oct 5, 2023

costin commented Oct 6, 2023

costin commented Oct 6, 2023

alex-spies left a comment

alex-spies Oct 6, 2023

costin Oct 6, 2023 •

edited

Loading

alex-spies Oct 6, 2023

ChrisHegarty Oct 6, 2023

ChrisHegarty left a comment

ChrisHegarty Oct 6, 2023

ChrisHegarty Oct 6, 2023

bpintea left a comment

bpintea Oct 6, 2023 •

edited

Loading

elasticsearchmachine commented Oct 6, 2023

ESQL: Page shouldn't close a block twice #100370

ESQL: Page shouldn't close a block twice #100370

Conversation

costin commented Oct 5, 2023

elasticsearchmachine commented Oct 5, 2023

elasticsearchmachine commented Oct 5, 2023

elasticsearchmachine commented Oct 5, 2023

nik9000 left a 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 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin commented Oct 6, 2023

costin commented Oct 6, 2023

alex-spies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin Oct 6, 2023 • edited Loading

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

bpintea left a comment

Choose a reason for hiding this comment

bpintea Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

elasticsearchmachine commented Oct 6, 2023

💔 Backport failed

costin Oct 6, 2023 •

edited

Loading

bpintea Oct 6, 2023 •

edited

Loading