Record column name for exceptions while writing frames in RowBasedFrameWriter #16130

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

processing/src/main/java/org/apache/druid/frame/write/FrameFieldWriterException.java

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

processing/src/main/java/org/apache/druid/frame/write/InvalidNullByteException.java

@@ -53,17 +50,16 @@ private InvalidNullByteException(
      @Nullable final Integer position
  )
  {
-    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.

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

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

extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java

+          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

...lti-stage-query/src/main/java/org/apache/druid/msq/indexing/error/FrameFieldWriterFault.java

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

...lti-stage-query/src/main/java/org/apache/druid/msq/indexing/error/FrameFieldWriterFault.java

+@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

...lti-stage-query/src/main/java/org/apache/druid/msq/indexing/error/FrameFieldWriterFault.java

@@ -49,7 +51,7 @@ public FrameFieldWriterFault(
    super(
        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

extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java

+                                   .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

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

@@ -299,11 +300,15 @@
                                      .column(signature.getColumnName(i))
                                      .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 bot found potential problems Mar 21, 2024

View reviewed changes

processing/src/main/java/org/apache/druid/frame/write/RowBasedFrameWriter.java Fixed Show fixed Hide 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 bot 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
75 of 85 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record column name for exceptions while writing frames in RowBasedFrameWriter #16130

Record column name for exceptions while writing frames in RowBasedFrameWriter #16130

gargvishesh commented Mar 15, 2024 •

edited

Loading

LakshSingla left a comment •

edited

Loading

LakshSingla Mar 15, 2024 •

edited

Loading

cryptoe Mar 19, 2024

gargvishesh Apr 1, 2024

LakshSingla Mar 15, 2024

cryptoe Mar 19, 2024

gargvishesh Apr 1, 2024

gargvishesh commented Apr 1, 2024

LakshSingla left a comment

LakshSingla Apr 3, 2024

gargvishesh Apr 4, 2024

LakshSingla Apr 3, 2024

gargvishesh Apr 4, 2024

LakshSingla Apr 4, 2024

gargvishesh Apr 4, 2024

LakshSingla Apr 4, 2024

gargvishesh Apr 4, 2024

LakshSingla Apr 4, 2024

gargvishesh Apr 4, 2024

LakshSingla left a comment

Record column name for exceptions while writing frames in RowBasedFrameWriter #16130

Record column name for exceptions while writing frames in RowBasedFrameWriter #16130

Conversation

gargvishesh commented Mar 15, 2024 • edited Loading

LakshSingla left a comment • edited Loading

Choose a reason for hiding this comment

LakshSingla Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gargvishesh commented Apr 1, 2024

LakshSingla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LakshSingla left a comment

Choose a reason for hiding this comment

gargvishesh commented Mar 15, 2024 •

edited

Loading

LakshSingla left a comment •

edited

Loading

LakshSingla Mar 15, 2024 •

edited

Loading