Skip to content

Handle exception for empty tables in elasticsearch#23850

Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom
K-Naga-Vivek:elasticsearch_fix
Nov 1, 2024
Merged

Handle exception for empty tables in elasticsearch#23850
tdcmeehan merged 1 commit intoprestodb:masterfrom
K-Naga-Vivek:elasticsearch_fix

Conversation

@K-Naga-Vivek
Copy link

@K-Naga-Vivek K-Naga-Vivek commented Oct 17, 2024

Add a catch block to handle the exception when a table in Elasticsearch does not have any columns. Previously, this situation would cause an internal error in Presto.

The code iterates through the mappings (columns) but lacks an exception handler for cases where an element is not found. So, catching the NoSuchElementException to throw the appropriate error.

Motivation and Context

#23849

Test Plan

Added the related testcase to the code change.

Tested and working as excepted.
image

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Elasticsearch Changes
* Improve handling of exceptions for empty tables in Elasticsearch :pr:`23850`

@K-Naga-Vivek K-Naga-Vivek requested a review from a team as a code owner October 17, 2024 03:30
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 17, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@steveburnett
Copy link
Contributor

Thanks for the release note entry! Nit suggestion to help it follow the Order of changes phrasing in the Release Notes Guidelines.

== RELEASE NOTES ==

Elasticsearch Changes
* Improve handling of exceptions for empty tables in Elasticsearch :pr:`23850`

throw new PrestoException(ELASTICSEARCH_INVALID_RESPONSE, e);
}
catch (NoSuchElementException e) {
throw new PrestoException(ELASTICSEARCH_QUERY_FAILURE, "Relation does not have any columns");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new PrestoException(ELASTICSEARCH_QUERY_FAILURE, "Relation does not have any columns");
throw new PrestoException(ELASTICSEARCH_QUERY_FAILURE, "Relation does not have any columns", e);

catch (IOException e) {
throw new PrestoException(ELASTICSEARCH_INVALID_RESPONSE, e);
}
catch (NoSuchElementException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain where this is thrown? I'm just wondering if it's because of the iteration on line 496.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, the issue occurs on the line mappings = mappings.elements().next();. When there are no properties, the flow enters the condition and proceeds to iterate to the next element, which leads to a NoSuchElementException.

Copy link
Contributor

@tdcmeehan tdcmeehan Oct 18, 2024

Choose a reason for hiding this comment

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

So instead of catching an exception, can you instead check if !hasNext() and throw?

@K-Naga-Vivek K-Naga-Vivek force-pushed the elasticsearch_fix branch 3 times, most recently from 6893594 to b00448a Compare October 19, 2024 02:28
Gracefully handle cases where Elasticsearch tables have no columns by
returning empty metadata instead of triggering an internal error in Presto.
@K-Naga-Vivek
Copy link
Author

@tdcmeehan, as suggested, I’ve updated the code to return an empty list instead of throwing an error when encountering tables without columns. This change resolves the failing test case and functions as expected. Now, if a SELECT * query is run on a table with no columns, the StatementAnalyzer throws an error: SELECT * not allowed from relation that has no columns as intended.

Could you please review the changes?

@tdcmeehan
Copy link
Contributor

Why does this connector throw at all if there are no columns? Can't it return a table with no columns?

@K-Naga-Vivek
Copy link
Author

The connector is currently set to return an empty list, but Presto has a check in the StatementAnalyzer that throws an error if a table has no columns.

@tdcmeehan
Copy link
Contributor

Can we let the exception occur in the analyzer, instead of throwing within the Elastic Search connector?

@K-Naga-Vivek
Copy link
Author

K-Naga-Vivek commented Nov 1, 2024

@tdcmeehan We are not throwing an exception within the connector. I've updated the code so that it now returns an empty list. The exception is now only thrown by the Analyzer.

@tdcmeehan
Copy link
Contributor

Got it. I was confused because I saw this message also in the Elastic client.

@tdcmeehan tdcmeehan merged commit c16711a into prestodb:master Nov 1, 2024
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
25 tasks
@tdcmeehan tdcmeehan added the from:IBM PR from IBM label Dec 13, 2024
@prestodb-ci prestodb-ci requested review from a team, namya28 and sh-shamsan and removed request for a team December 13, 2024 15:33
@prestodb-ci
Copy link
Contributor

Saved that user @K-Naga-Vivek is from IBM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:IBM PR from IBM

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments