Skip to content
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

Fix incorrect check of InvalidFieldException to InvalidFieldFault while generating MSQ Error Report #16273

Conversation

gargvishesh
Copy link
Contributor

@gargvishesh gargvishesh commented Apr 12, 2024

InvalidFieldFault is incorrectly checked as InvalidFieldException in mapQueryColumnNameToOutputColumnName. Fix it to the correct check. Also added test for the generated exception as part of RowBasedFrameWriterTest.

Verified the generated MSQErrorReport by raising an empty RuntimeException inside StringFieldWriter.writeTo() in a custom build.
image

@github-actions github-actions bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Apr 12, 2024
@LakshSingla
Copy link
Contributor

LakshSingla commented Apr 12, 2024

Can we add any unit tests here? It feels weird that we have to test it in a separate custom build to generate the exception. Can you mention where you are throwing an exception to test this flow? It'd be prudent to add a unit test that throws in the same flow to test this exception.

@gargvishesh
Copy link
Contributor Author

Have added RowBasedFrameWriterTest for the generated InvalidFieldException.

Comment on lines 239 to 240
@VisibleForTesting
boolean writeData()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should revert this change, and look for a way to generate the exception using the class's public methods (#11848 (comment)), if possible. Since the code coverage is still not satisfied, I think we can remove the new test added (since that isn't testing the actual code in the ControllerImpl where everything is bubbled up to) and merge it as is, since it's a fairly low risk change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment and the reference. Using a public method now instead.

@LakshSingla
Copy link
Contributor

Failures are unrelated. Thanks for the PR 🚀

@LakshSingla LakshSingla merged commit 173a206 into apache:master Apr 22, 2024
23 of 29 checks passed
@adarshsanjeev adarshsanjeev added this to the 30.0.0 milestone May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants