Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/126411.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 126411
summary: Fix usage of already released null block in `ValueSourceReaderOperator`
area: ES|QL
type: bug
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,12 @@ public BlockLoader.Builder nulls(int expectedCount) {

@Override
public Block constantNulls() {
if (nullBlock == null) {
// Avoid creating a new null block if we already have one.
// However, downstream operators take ownership of the null block we hand to them and may close it, forcing us to create a new
// one.
// This could be avoided altogether if this factory itself kept a reference to the null block, but we'd have to also ensure
// to release this block once the factory is not used anymore.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the factory itself would have to be Releasable. FWIW, that's not really a big deal. I think I'd prefer it to this fix to be honest. I'm kind of ok with whichever though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if you are trying to get this merged quickly I'd be fine with this solution - especially if you are willing to try the other in a followup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also prefer making this Releasable. It's easy to do, I'll push a commit with the change.

Wrong implementations of .close() or forgetting to incref when handing out blocks actually nicely show up as test failures already in the existing tests, so I think this is guarding better against regressions.

if (nullBlock == null || nullBlock.isReleased()) {
nullBlock = factory.newConstantNullBlock(pageSize);
} else {
nullBlock.incRef();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -947,7 +947,12 @@ public enum Cap {
/**
* Supercedes {@link Cap#MAKE_NUMBER_OF_CHANNELS_CONSISTENT_WITH_LAYOUT}.
*/
FIX_REPLACE_MISSING_FIELD_WITH_NULL_DUPLICATE_NAME_ID_IN_LAYOUT;
FIX_REPLACE_MISSING_FIELD_WITH_NULL_DUPLICATE_NAME_ID_IN_LAYOUT,

/**
* Fix https://github.com/elastic/elasticsearch/issues/125850
*/
FIX_DOUBLY_RELEASED_NULL_BLOCKS_IN_VALUESOURCEREADER;

private final boolean enabled;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -335,3 +335,58 @@
esql.query:
body:
query: 'FROM test_grok | KEEP name | WHERE last_name == "Facello" | EVAL name = concat("1 ", last_name) | GROK name "%{NUMBER:foo} %{WORD:foo}"'
---
"union types with null blocks from missing fields #125850":
- requires:
test_runner_features: [allowed_warnings_regex, capabilities]
capabilities:
- method: POST
path: /_query
parameters: []
capabilities: [fix_doubly_released_null_blocks_in_valuesourcereader]
reason: "fixed handing out already closed null block references in ValueSourceReader"
- do:
indices.create:
index: test1
body:
mappings:
properties:
truefalse1 :
type : boolean
truefalse2 :
type: boolean
- do:
indices.create:
index: test2
body:
mappings:
properties:
truefalse1 :
type : keyword
truefalse2 :
type: keyword
- do:
bulk:
refresh: true
body:
- { "index": { "_index": "test1" } }
- { "truefalse1": null}
- { "index": { "_index": "test2" } }
- { "truefalse1": null }

- do:
allowed_warnings_regex:
- "No limit defined, adding default limit of \\[.*\\]"

esql.query:
body:
query: 'FROM test* | eval t1 = truefalse1::boolean, t2 = truefalse2::boolean | keep t1, t2'
- match: { columns.0.name: t1 }
- match: { columns.0.type: boolean }
- match: { columns.1.name: t2 }
- match: { columns.1.type: boolean }
- length: { values: 2 }
- match: { values.0.0: null }
- match: { values.0.1: null }
- match: { values.1.0: null }
- match: { values.1.1: null }
Loading