Skip to content

ESQL: Fix validation NPE in Enrich and add extra @Nullable annotations #128260

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

Merged
merged 2 commits into from
May 23, 2025

Conversation

ivancea
Copy link
Contributor

@ivancea ivancea commented May 21, 2025

Fixes #126253
Fixes #126297

inputDataType may be null when in mixed cluster (<8.14). So validateTypes() should take that into account. Similar fix to #116583

@ivancea ivancea requested review from craigtaverner and nik9000 May 21, 2025 15:16
@ivancea ivancea added >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.19.0 v9.1.0 v9.0.3 labels May 21, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine
Copy link
Collaborator

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

@@ -128,8 +129,8 @@ protected LookupResponse readLookupResponse(StreamInput in, BlockFactory blockFa
return new LookupResponse(in, blockFactory);
}

private static void validateTypes(DataType inputDataType, MappedFieldType fieldType) {
if (fieldType instanceof RangeFieldMapper.RangeFieldType rangeType) {
private static void validateTypes(@Nullable DataType inputDataType, MappedFieldType fieldType) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The @Nullable annotations are showing a warning here as expected. But they're not showing a warning on method calls. Not perfect, but well. I think they may still help (?)

Copy link
Contributor

@craigtaverner craigtaverner left a comment

Choose a reason for hiding this comment

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

LGTM, although I'm assuming the annotations don't change anything and it's all in the inputDataType == null check.

@ivancea ivancea merged commit 0c3d47d into elastic:main May 23, 2025
18 checks passed
@ivancea ivancea deleted the esql-npe-enrich-validate branch May 23, 2025 14:05
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.19 Commit could not be cherrypicked due to conflicts
9.0 Commit could not be cherrypicked due to conflicts

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

ivancea added a commit to ivancea/elasticsearch that referenced this pull request May 26, 2025
elastic#128260)

Fixes elastic#126253
Fixes elastic#126297

`inputDataType` may be null when in mixed cluster (<8.14). So `validateTypes()` should take that into account. Similar fix to elastic#116583
ivancea added a commit to ivancea/elasticsearch that referenced this pull request May 26, 2025
elastic#128260)

Fixes elastic#126253
Fixes elastic#126297

`inputDataType` may be null when in mixed cluster (<8.14). So `validateTypes()` should take that into account. Similar fix to elastic#116583
elasticsearchmachine pushed a commit that referenced this pull request May 26, 2025
#128260) (#128452)

Fixes #126253
Fixes #126297

`inputDataType` may be null when in mixed cluster (<8.14). So `validateTypes()` should take that into account. Similar fix to #116583
elasticsearchmachine pushed a commit that referenced this pull request May 26, 2025
#128260) (#128453)

Fixes #126253
Fixes #126297

`inputDataType` may be null when in mixed cluster (<8.14). So `validateTypes()` should take that into account. Similar fix to #116583
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.19.0 v9.0.3 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] EsqlClientYamlIT test {p0=esql/61_enrich_ip/IP strings} failing [CI] EsqlClientYamlIT class failing
3 participants