Column name in parse exceptions #16529

TSFenwick · 2024-05-31T19:24:42Z

Makes the column name or dimension name be more apparent in parse exceptions

Description

Often times parse exception occur that end up being user visible, but they don't offer any information on how to remediate them. This change adds more information to allow a user to fix the actual issue.

For example in the report of a streaming job the details changes from

      {
        "input": "{isRobot=false, channel=#sv.wikipedia, timestamp=2024-05-30T21:28:18.343Z, flags=M, isUnpatrolled=false, page=Fredrik Johan Leth, diffUrl=https://sv.wikipedia.org/w/index.php?diff=36127609&oldid=34977802, added=41, comment=tog bort [[Kategori:KDDO1gr]]; lade till [[Kategori:Kommendörer av första graden av Dannebrogorden]] med [[Wikipedia:Hotcat|Hotcat]], commentLength=132, isNew=false, isMinor=true, delta=, isAnonymous=false, user=Portunes, deltaBucket=0.0, deleted=0, namespace=Main}",
        "errorType": "processedWithError",
        "details": [
          "could not convert value [] to long"
        ],
        "timeOfExceptionMillis": 1717181630058
      }

to

      {
        "input": "{isRobot=false, channel=#sv.wikipedia, timestamp=2024-05-30T21:28:18.343Z, flags=M, isUnpatrolled=false, page=Fredrik Johan Leth, diffUrl=https://sv.wikipedia.org/w/index.php?diff=36127609&oldid=34977802, added=41, comment=tog bort [[Kategori:KDDO1gr]]; lade till [[Kategori:Kommendörer av första graden av Dannebrogorden]] med [[Wikipedia:Hotcat|Hotcat]], commentLength=132, isNew=false, isMinor=true, delta=, isAnonymous=false, user=Portunes, deltaBucket=0.0, deleted=0, namespace=Main}",
        "errorType": "processedWithError",
        "details": [
          "could not convert value [] to long for dimension [delta]"
        ],
        "timeOfExceptionMillis": 1717181630058
      }

One thing i thought of while doing this is I think it would make sense to make the ParseException actually contain another parameter to have the dimension or key that failed on parsing rather than just the input. But i think that can be left to another PR.

Release note

Key changed/added classes in this PR

processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java

This PR has:

…rrors with dimension name

processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java

abhishekrb19

Looks good to me overall. I left a few comments related to error messages and code formatting, and a question related to Nullable annotations.

Thanks for the QoL improvement @TSFenwick!

processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java

abhishekrb19 · 2024-06-07T17:40:54Z

processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java

-      if (reportParseExceptions && ret == null) {
-        throw new ParseException((String) valObj, "could not convert value [%s] to float", valObj);
+  @Nullable
+  public static Float convertObjectToFloat(@Nullable Object valObj, @Nullable String fieldName)


Question on the @Nullable annotation: this new overloaded function appears to be called only from SqlResults similar to convertObjectToLong() where the fieldName doesn't have a @Nullable annotation. I don't think fieldName can be nullable in this context, right? If that's correct, please remove the annotation here; otherwise, update convertObjectToLong's signature above to reflect that

processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java

abhishekrb19

+1 after CI!

abhishekrb19 · 2024-06-11T23:23:34Z

processing/src/main/java/org/apache/druid/segment/FloatDimensionIndexer.java

@@ -52,6 +52,16 @@ public EncodedKeyComponent<Float> processRowValsToUnsortedEncodedKeyComponent(@N
    return new EncodedKeyComponent<>(f, Float.BYTES);


For this method override, just return processRowValsToUnsortedEncodedKeyComponent(dimValues, reportParseExceptions, null) to avoid duplication of code

abhishekrb19 · 2024-06-11T23:24:09Z

processing/src/main/java/org/apache/druid/segment/FloatDimensionIndexer.java

+  @Override
+  public EncodedKeyComponent<Float> processRowValsToUnsortedEncodedKeyComponent(@Nullable Object dimValues, boolean reportParseExceptions, @Nullable String dimension)
+  {
+    Float l = DimensionHandlerUtils.convertObjectToFloat(dimValues, reportParseExceptions, dimension);


super nit: variable rename for float: Float l -> Float f

abhishekrb19 · 2024-06-11T23:33:07Z

processing/src/main/java/org/apache/druid/segment/DimensionIndexer.java

@@ -137,6 +137,15 @@ EncodedKeyComponent<EncodedKeyComponentType> processRowValsToUnsortedEncodedKeyC
      boolean reportParseExceptions
  );

+  default EncodedKeyComponent<EncodedKeyComponentType> processRowValsToUnsortedEncodedKeyComponent(


Since this is an interface method, it could use a brief javadoc:

Suggested change

default EncodedKeyComponent<EncodedKeyComponentType> processRowValsToUnsortedEncodedKeyComponent(

/**

* Similar to {@link #processRowValsToUnsortedEncodedKeyComponent}, but with an optional

* dimension parameter to provide more context for error handling where applicable.

*/

default EncodedKeyComponent<EncodedKeyComponentType> processRowValsToUnsortedEncodedKeyComponent(

abhishekrb19 · 2024-06-11T23:35:23Z

...kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/KafkaIndexTaskTest.java

@@ -1634,8 +1634,8 @@ public void testMultipleParseExceptionsSuccess() throws Exception

    List<String> expectedMessages = Arrays.asList(
        "Unable to parse value[notanumber] for field[met1]",
-        "could not convert value [notanumber] to float",
-        "could not convert value [notanumber] to long",
+        "could not convert value [notanumber] to float for dimension [dimFloat]",


I think CI is failing because these test expectations need to be updated?

clintropolis · 2024-06-12T00:19:03Z

processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndex.java

@@ -608,7 +608,7 @@ IncrementalIndexRowResult toIncrementalIndexRow(InputRow row)
        Object dimsKey = null;
        try {
          final EncodedKeyComponent<?> encodedKeyComponent
-              = indexer.processRowValsToUnsortedEncodedKeyComponent(row.getRaw(dimension), true);
+              = indexer.processRowValsToUnsortedEncodedKeyComponent(row.getRaw(dimension), true, dimension);


hmm, do we really need to pass this as an argument for every value of every row? maybe it would be better to just initialize the dimension indexers with their name instead.

Alternatively, we are already catching ParseException, why not add a way to decorate it with an additional column name.

I guess i'm just being cautious about small overhead of additional arguments and the default method impl adding up since this is a pretty hot function

@clintropolis I think i could do that. Lemme get this current PR passing. then i'll attempt that

@clintropolis i was able to initialize the dimensionIndexers with the name

cool, that seems better.

thinking a bit more on it i think having a way to decorate an existing ParseException with a column name might actually have been a little cleaner and even less disruptive since we are already catching them in a place with the column name, https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndex.java#L616 without being dependent on dimension indexer implementation, but this way seems ok too

…imensionHandlerUtils

clintropolis · 2024-06-12T02:16:58Z

processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java

      case STRING:
        return convertObjectToString(obj);
      case ARRAY:
        return coerceToObjectArrayWithElementCoercionFunction(
            obj,
-            x -> DimensionHandlerUtils.convertObjectToType(x, type.getElementType())
+            x -> DimensionHandlerUtils.convertObjectToType(x, type.getElementType(), false, fieldName)


this should probably pass reportParseExceptions instead of false?

yeah. I set it to false to preserve how it was working before. but it wouldn't hurt to have it pass reportParseExceptions instead

clintropolis · 2024-06-12T02:24:53Z

processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndex.java

@@ -608,7 +608,7 @@ IncrementalIndexRowResult toIncrementalIndexRow(InputRow row)
        Object dimsKey = null;
        try {
          final EncodedKeyComponent<?> encodedKeyComponent
-              = indexer.processRowValsToUnsortedEncodedKeyComponent(row.getRaw(dimension), true);
+              = indexer.processRowValsToUnsortedEncodedKeyComponent(row.getRaw(dimension), true, dimension);


cool, that seems better.

thinking a bit more on it i think having a way to decorate an existing ParseException with a column name might actually have been a little cleaner and even less disruptive since we are already catching them in a place with the column name, https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndex.java#L616 without being dependent on dimension indexer implementation, but this way seems ok too

clintropolis · 2024-06-12T02:27:09Z

processing/src/main/java/org/apache/druid/segment/StringDimensionIndexer.java

@@ -155,7 +155,7 @@ public EncodedKeyComponent<int[]> processRowValsToUnsortedEncodedKeyComponent(@N
  /**
   * Estimates size of the given key component.
   * <p>
-   * Deprecated method. Use {@link #processRowValsToUnsortedEncodedKeyComponent(Object, boolean)}
+   * Deprecated method. Use {@link #processRowValsToUnsortedEncodedKeyComponent(Object, boolean, String)}


this needs reverted i think

findingrish · 2024-06-12T14:49:31Z

processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java

+          final String message;
+          if (fieldName != null) {
+            message = StringUtils.nonStrictFormat(
+                "Could not convert value [%s] to float for dimension [%s].",


nit: The exception message in the if/else block seem to differ by for dimension [%s]. Instead of duplicating the message does it make sense to have a single template?

…onvertToType

…rse-exceptions

TSFenwick added 2 commits May 28, 2024 12:45

first pass

f940e20

more changes

7d114bb

github-actions bot added Area - Querying Area - Segment Format and Ser/De labels May 31, 2024

fix tests and formatting

164b6e2

github-actions bot added the Area - Ingestion label May 31, 2024

fix kinesis failing tests

da15cad

github-actions bot added the Area - Streaming Ingestion label May 31, 2024

TSFenwick added 5 commits May 31, 2024 18:42

fix kafka tests

d63d65d

add dimension name to float parse errors

37f81c9

double and convertToType handling of dimensionName can report parse e…

f164c6d

…rrors with dimension name

fix checkstyle issue

49aae0e

fix tests

6603c9b

TSFenwick marked this pull request as ready for review June 5, 2024 22:20

TSFenwick commented Jun 6, 2024

View reviewed changes

processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java Outdated Show resolved Hide resolved

TSFenwick added 3 commits June 5, 2024 18:48

more cases to have better parse exception messages

9397ad3

fix test

1e0864a

fix tests

e7466c3

abhishekrb19 reviewed Jun 7, 2024

View reviewed changes

TSFenwick added 3 commits June 7, 2024 14:50

partially address comments

b3ad7a6

annotate method parameter with nullable

a58a2a5

address comments

cf2c494

TSFenwick requested a review from abhishekrb19 June 11, 2024 19:46

abhishekrb19 approved these changes Jun 11, 2024

View reviewed changes

clintropolis reviewed Jun 12, 2024

View reviewed changes

fix tests

85faa15

github-actions bot added the Area - Batch Ingestion label Jun 12, 2024

TSFenwick added 3 commits June 11, 2024 19:06

let float, double, long dimensionIndexer pass dimensionName down to d…

52b9555

…imensionHandlerUtils

fix compilation error and clean up formatting

f1ea266

clean up whitespace

6b08fa0

clintropolis reviewed Jun 12, 2024

View reviewed changes

findingrish reviewed Jun 12, 2024

View reviewed changes

TSFenwick added 6 commits June 13, 2024 15:52

address feedback. undo change, pass down report parse exception for c…

7e98f52

…onvertToType

fix test

0ac0d59

Merge remote-tracking branch 'upstream/master' into column-name-in-pa…

754b285

…rse-exceptions

Merge remote-tracking branch 'upstream/master' into column-name-in-pa…

eb97391

…rse-exceptions

Merge remote-tracking branch 'upstream/master' into column-name-in-pa…

c398798

…rse-exceptions

Merge remote-tracking branch 'upstream/master' into column-name-in-pa…

bfecd95

…rse-exceptions

TSFenwick force-pushed the column-name-in-parse-exceptions branch from 8f6d8f9 to bfecd95 Compare June 25, 2024 17:01

clintropolis approved these changes Jun 25, 2024

View reviewed changes

abhishekrb19 merged commit 52c9929 into apache:master Jun 25, 2024
88 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Column name in parse exceptions #16529

Column name in parse exceptions #16529

TSFenwick commented May 31, 2024 •

edited

Loading

abhishekrb19 left a comment

abhishekrb19 Jun 7, 2024

abhishekrb19 left a comment

abhishekrb19 Jun 11, 2024

abhishekrb19 Jun 11, 2024

abhishekrb19 Jun 11, 2024

abhishekrb19 Jun 11, 2024

clintropolis Jun 12, 2024 •

edited

Loading

TSFenwick Jun 12, 2024

TSFenwick Jun 12, 2024

clintropolis Jun 12, 2024

clintropolis Jun 12, 2024

TSFenwick Jun 12, 2024

clintropolis Jun 12, 2024

clintropolis Jun 12, 2024

findingrish Jun 12, 2024

		@@ -52,6 +52,16 @@ public EncodedKeyComponent<Float> processRowValsToUnsortedEncodedKeyComponent(@N
		return new EncodedKeyComponent<>(f, Float.BYTES);

Column name in parse exceptions #16529

Column name in parse exceptions #16529

Conversation

TSFenwick commented May 31, 2024 • edited Loading

Description

Release note

Key changed/added classes in this PR

abhishekrb19 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishekrb19 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

clintropolis Jun 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TSFenwick commented May 31, 2024 •

edited

Loading

clintropolis Jun 12, 2024 •

edited

Loading