Record column name for exceptions while writing frames in RowBasedFrameWriter by gargvishesh · Pull Request #16130 · apache/druid

gargvishesh · 2024-03-15T05:46:26Z

Current Runtime Exceptions generated while writing frames only include the exception itself without including the name of the column they were encountered in, for e.g.

java.lang.RuntimeException: java.lang.ClassCastException: class java.lang.Long cannot be cast to class java.lang.String (java.lang.Long and java.lang.String are in module java.base of loader 'bootstrap')

Start including additional details for easier debugging.

New Exception:
[InvalidFieldError](https://druid.apache.org/docs/latest/multi-stage-query/reference.html#error_InvalidFieldError): Error[class java.lang.Long cannot be cast to class java.lang.String (java.lang.Long and java.lang.String are in module java.base of loader 'bootstrap')] while writing field for source[external input source: LocalInputSource{baseDir="null", filter=null, files=[foo]}], rowNumber[1], column[arrayString].

Tested the generated exception by raising it explicitly in a custom build.

…ptions while writing frames

LakshSingla

Thanks for the PR!

I have added a few comments about the relevance of the new error class.
Can you please add a test case where the exception gets generated?
Also, what does it mean for MSQ, should MSQ retry if it encounters such exceptions or not? It seems like retrying shouldn't help here, but per my understanding, MSQ will still treat errors like these as UnknownFaults, and will retry. We should update that logic in MSQ (check how we do it for InvalidNullByte)

LakshSingla · 2024-03-15T09:30:12Z

I doubt that we need this class anywhere. Can't we use a generic RuntimeException with the proper error message? Or better yet, use DruidException, so that we can analyze the message.

I think we can make a MSQ fault out of this. That way we donot have to adjust any retry logic.

Converting FrameFieldWriterException now to an MSQFault type

LakshSingla · 2024-03-15T09:32:37Z

  )
  {
-    super(StringUtils.format(
+    super(column, StringUtils.format(


Feels weird that we have this extra layer of indirection, to what will essentially call the RuntimeException. If we don't extract the relevant fields (.getColumn()) from the FrameFieldWriterException anywhere, or use check for it's instanceof, then I suppose we can get rid of it.

Converting to MSQFault now for which the specific exception class is required. InvalidNullByteException now extends RuntimeException directly.

gargvishesh · 2024-04-01T05:29:33Z

Can you please add a test case where the exception gets generated?

Tried multiple routes incl insert via external inline tables and insert via joins, but an error isn't generated. Therefore not adding the test case.

Also, what does it mean for MSQ, should MSQ retry if it encounters such exceptions or not? It seems like retrying shouldn't help here, but per my understanding, MSQ will still treat errors like these as UnknownFaults, and will retry. We should update that logic in MSQ (check how we do it for InvalidNullByte)

The conversion to FrameFieldWriterFault now puts it in the category of non-retryable errors.

LakshSingla

minor comment, overall the changes LGTM

LakshSingla · 2024-04-03T13:34:44Z

+          FrameFieldWriterException.builder()
+                                   .source(ffre.getSource())
+                                   .rowNumber(ffre.getRowNumber())
+                                   .column(ffre.getColumn())
+                                   .errorMsg(ffre.getErrorMsg())
+                                   .build(),


Let's use the normal constructor since we are using all the parameters in a single place. Invalid null byte exception has the builder because it populates different information at different places up the call stack.

IIUC, the ask is to remove the builder from FrameFieldWriterException. But we do build it progressively in ScanQueryFrameProcessor.

LakshSingla · 2024-04-03T13:35:48Z

Let's add this class to MSQFaultsSerdeTest for completeness.

Done. I've also added the description of the fault in reference.md and removed the term frame from the fault code and log message since that is user-facing.

LakshSingla · 2024-04-04T07:32:44Z

+@JsonTypeName(FrameFieldWriterFault.CODE)
+public class FrameFieldWriterFault extends BaseMSQFault
+{
+  static final String CODE = "FieldWriterError";


This doesn't match the class name, can you please update the class as well? Also, wrt the naming convention of other faults, we don't have "Error" in the suffix. I think something like "InvalidFieldFault" might be more suitable, and the name could be "InvalidField". I am open to other naming conventions as well.

Went with your suggestion. We do have UnknownFault->UnknownError though, but InvalidField sounded fine.

LakshSingla · 2024-04-04T07:34:13Z

        CODE,
        StringUtils.format(
-            "Error[%s] while writing frame for source[%s], rowNumber[%d] column[%s]",
+            "Error[%s] while writing a field for source[%s], rowNumber[%d], column[%s].",


nit: Duplicate error message as FrameFieldWriterException , can extract the format string out. It will make the updates easier.

Now propagate the RE's message directly to the Fault class.

LakshSingla · 2024-04-04T07:35:42Z

+                                   .source(ffre.getSource())
+                                   .rowNumber(ffre.getRowNumber())
+                                   .column(ffre.getColumn())
+                                   .errorMsg(ffre.getErrorMsg())


We should also add the problematic field.

The field value isn't available at the place the exception starts -- which is RowBasedFrameWriter#writeDataUsingFieldWriters

                                      .build();
      }
+      catch (ParseException pe) {
+        throw Throwables.propagate(pe);


LakshSingla

LGTM. Failures are code coverage checks - but there isn't a known dataset that will trigger this. Since the changes are isolated, they shouldn't affect the current code.

gargvishesh added 3 commits March 15, 2024 10:46

Add FrameFieldWriterException and use it to annotate all runtime exce…

c403739

…ptions while writing frames

Remove redundant generic exception catch block

8b31dfb

Add licence

3fadc2c

LakshSingla reviewed Mar 15, 2024

View reviewed changes

Add fault class, modify exception class, and add conversion code

1e33b8c

github-actions Bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Mar 21, 2024

Pass-through ParseException to avoid convoluting the exception.

f185bc4

github-advanced-security AI found potential problems Mar 21, 2024

View reviewed changes

Comment thread processing/src/main/java/org/apache/druid/frame/write/RowBasedFrameWriter.java Fixed

Fix checkstyle error

99057e9

LakshSingla reviewed Apr 3, 2024

View reviewed changes

Address review comments

acdd50a

github-actions Bot added the Area - Documentation label Apr 4, 2024

LakshSingla reviewed Apr 4, 2024

View reviewed changes

gargvishesh added 4 commits April 4, 2024 13:31

Change fault class name

9c55e3d

Change to InvalidFieldException

7aec0cc

Resolve checkstyle issues

1d1a32f

Resolve checkstyle again

bc96666

github-advanced-security AI found potential problems Apr 4, 2024

View reviewed changes

fix for failing test

2ea06a7

gargvishesh requested a review from LakshSingla April 5, 2024 12:11

LakshSingla approved these changes Apr 9, 2024

View reviewed changes

gargvishesh added 2 commits April 9, 2024 10:57

Merge branch 'master' into 43059-col-name-in-framewriter-exception

1c9d0c3

Merge branch 'master' into 43059-col-name-in-framewriter-exception

f99d360

LakshSingla merged commit 9a4fb58 into apache:master Apr 9, 2024

adarshsanjeev added this to the 30.0.0 milestone May 6, 2024

adarshsanjeev mentioned this pull request May 28, 2024

[DRAFT] 30.0.0 release notes #16505

Closed

Conversation

gargvishesh commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LakshSingla left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LakshSingla Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gargvishesh commented Apr 1, 2024

Uh oh!

LakshSingla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check notice

LakshSingla left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gargvishesh commented Mar 15, 2024 •

edited

Loading

LakshSingla left a comment •

edited

Loading

LakshSingla Mar 15, 2024 •

edited

Loading