Skip to content

[ES|QL] To_DatePeriod and To_TimeDuration return better error messages on union_type fields#114934

Merged
fang-xing-esql merged 7 commits intoelastic:mainfrom
fang-xing-esql:interval-union-type-error-message
Nov 12, 2024
Merged

[ES|QL] To_DatePeriod and To_TimeDuration return better error messages on union_type fields#114934
fang-xing-esql merged 7 commits intoelastic:mainfrom
fang-xing-esql:interval-union-type-error-message

Conversation

@fang-xing-esql
Copy link
Copy Markdown
Member

@fang-xing-esql fang-xing-esql commented Oct 16, 2024

Resolves: #112668

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Oct 16, 2024

Hi @fang-xing-esql, I've created a changelog YAML for you.

@fang-xing-esql fang-xing-esql marked this pull request as ready for review October 16, 2024 19:30
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 16, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Oct 16, 2024

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

alex-spies
alex-spies reviewed Oct 18, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

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

Thanks @fang-xing-esql !

This nearly LGTM and I have mostly minor comments; the only, slightly not-so-minor remark is that we shouldn't use UnsupportedAttribute here as that has a pretty specific meaning and treatment in both the verifier and compute engine.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java Outdated
new UnsupportedEsField(imf.getName(), types),
unresolvedMessage
);
return fcf.replaceChildren(Collections.singletonList(ua));
Copy link
Copy Markdown
Contributor

@alex-spies alex-spies Oct 18, 2024

Choose a reason for hiding this comment

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

bikeshed: we could probably also replace the whole convert function by an UnresolvedAttribute (with the custom message from above); probably irrelevant for the final verification, though.

Copy link
Copy Markdown
Member Author

@fang-xing-esql fang-xing-esql Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd prefer to keep the function name, not revert it back to UnresolvedFunction, as ResolveFunctions has done its job, and it is correct. The conversion function's name has been resolved by ResolveFunctions, however its children haven't been resolved yet.

@bpintea bpintea mentioned this pull request Oct 18, 2024
Closed
@fang-xing-esql fang-xing-esql added the auto-backport Automatically create backport pull requests when merged label Oct 31, 2024
@fang-xing-esql
Copy link
Copy Markdown
Member Author

fang-xing-esql commented Oct 31, 2024

Thanks for reviewing @alex-spies ! The comments have been addressed in the last commit.

@fang-xing-esql
Copy link
Copy Markdown
Member Author

fang-xing-esql commented Oct 31, 2024

@elasticmachine update branch

@fang-xing-esql
Copy link
Copy Markdown
Member Author

fang-xing-esql commented Nov 7, 2024

@elasticmachine update branch

bpintea
bpintea approved these changes Nov 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

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

Lgtm

@fang-xing-esql fang-xing-esql merged commit eb6d47f into elastic:main Nov 12, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Nov 12, 2024

💔 Backport failed

The backport operation could not be completed due to the following error:

An unexpected error occurred when attempting to backport this PR.

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

@fang-xing-esql fang-xing-esql mentioned this pull request Nov 12, 2024
Merged
@fang-xing-esql
Copy link
Copy Markdown
Member Author

fang-xing-esql commented Nov 12, 2024

💚 All backports created successfully

Status Branch Result
✅ 8.x

Questions ?

Please refer to the Backport tool documentation

fang-xing-esql added a commit to fang-xing-esql/Elasticsearch that referenced this pull request Nov 12, 2024
@fang-xing-esql
[ES|QL] To_DatePeriod and To_TimeDuration return better error message…
2041611 
…s on union_type fields (elastic#114934)

* better error messages with union_type fields

(cherry picked from commit eb6d47f)
elasticsearchmachine pushed a commit that referenced this pull request Nov 12, 2024
@fang-xing-esql
[ES|QL] To_DatePeriod and To_TimeDuration return better error message…
8dc9b7b 
…s on union_type fields (#114934) (#116631)

* better error messages with union_type fields

(cherry picked from commit eb6d47f)
jozala pushed a commit that referenced this pull request Nov 13, 2024
@fang-xing-esql @jozala
[ES|QL] To_DatePeriod and To_TimeDuration return better error message…
796882d 
…s on union_type fields (#114934)

* better error messages with union_type fields
afoucret pushed a commit to afoucret/elasticsearch that referenced this pull request Nov 14, 2024
@fang-xing-esql
[ES|QL] To_DatePeriod and To_TimeDuration return better error message…
9cb4172 
…s on union_type fields (elastic#114934)

* better error messages with union_type fields
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
@fang-xing-esql @alexey-ivanov-es
[ES|QL] To_DatePeriod and To_TimeDuration return better error message…
5597b20 
…s on union_type fields (elastic#114934)

* better error messages with union_type fields
@fang-xing-esql fang-xing-esql deleted the interval-union-type-error-message branch December 5, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alex-spies alex-spies alex-spies left review comments

@bpintea bpintea bpintea approved these changes

@craigtaverner craigtaverner Awaiting requested review from craigtaverner

Assignees

No one assigned

Labels

:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged backport pending >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.17.0 v9.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ES|QL] Return correct correct message when a union type field is provided to to_dateperiod or to_timeduration

5 participants

@fang-xing-esql @elasticsearchmachine @bpintea @alex-spies @elasticmachine