Skip to content

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

Merged
LakshSingla merged 4 commits intoapache:masterfrom
gargvishesh:fix-invalid-field-fault-checked-as-exception
Apr 22, 2024
Merged

Fix incorrect check of InvalidFieldException to InvalidFieldFault while generating MSQ Error Report#16273
LakshSingla merged 4 commits intoapache:masterfrom
gargvishesh:fix-invalid-field-fault-checked-as-exception

Conversation

@gargvishesh
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Have added RowBasedFrameWriterTest for the generated InvalidFieldException.

Comment on lines +239 to +240
@VisibleForTesting
boolean writeData()
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.

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
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 the comment and the reference. Using a public method now instead.

@LakshSingla
Copy link
Copy Markdown
Contributor

Failures are unrelated. Thanks for the PR 🚀

@LakshSingla LakshSingla merged commit 173a206 into apache:master Apr 22, 2024
@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