Skip to content

Convert errors based on implicit type conversion in multi value arrays to parse exception in MSQ#13366

Merged
cryptoe merged 7 commits intoapache:masterfrom
LakshSingla:extern-object-list-regression
Nov 29, 2022
Merged

Convert errors based on implicit type conversion in multi value arrays to parse exception in MSQ#13366
cryptoe merged 7 commits intoapache:masterfrom
LakshSingla:extern-object-list-regression

Conversation

@LakshSingla
Copy link
Copy Markdown
Contributor

@LakshSingla LakshSingla commented Nov 15, 2022

Description

Ingesting a multi-value column which was originally expecting a string array but received an object array throws a cryptic error message due to a failed type conversion that happens in the FrameWriters. This PR aims to convert those catch those error messages and throw them as ParseExceptions.



This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@LakshSingla LakshSingla force-pushed the extern-object-list-regression branch from 3e80717 to d9cfe98 Compare November 15, 2022 05:59
@kfaraz kfaraz added Error handling Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Nov 21, 2022
Copy link
Copy Markdown
Contributor

@adarshsanjeev adarshsanjeev left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. LGTM!

@cryptoe cryptoe added this to the 25.0 milestone Nov 22, 2022
partitionBoostVirtualColumn.setValue(partitionBoostVirtualColumn.getValue() + 1);
}
}
catch (Exception e) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Woudn't FrameRowTooLargeException be caught here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. I have reduced the surface of the try..catch block so as to not obstruct any other exceptions


while (!cursor.isDone()) {
if (!frameWriter.addSelection()) {
boolean addedSelection;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should also make this change in all the leaf processors.
So it can be the scan processor and the GroupByPreShuffleFrameProcessor. So this change might be required there as well.
Or you can send the common code to the superclass BaseLeafFrameProcessor.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the test case and made the change. Instead of the individual processors, I have added the handling in the individual frame writers so that we won't have to add a block in every processor.

Copy link
Copy Markdown
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

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

LGTM!!
Thanks for the contribution @LakshSingla.

@cryptoe cryptoe merged commit 4ed6255 into apache:master Nov 29, 2022
LakshSingla added a commit to LakshSingla/druid that referenced this pull request Nov 29, 2022
…s to parse exception in MSQ (apache#13366)

* initial commit

* fix test

* push the json changes

* reduce the area of the try..catch

* Trigger Build

* review
cryptoe pushed a commit that referenced this pull request Nov 30, 2022
…s to parse exception in MSQ (#13366) (#13454)

* initial commit

* fix test

* push the json changes

* reduce the area of the try..catch

* Trigger Build

* review
@LakshSingla LakshSingla deleted the extern-object-list-regression branch July 11, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 Error handling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants