Logging the fieldName in the coerce exceptions by pranavbhole · Pull Request #14483 · apache/druid

pranavbhole · 2023-06-24T01:30:38Z

Description

This PR is improving the logging in coerce exceptions, coerce is currently eating some exceptions esp on JSON and primitive array parsing. This change is adding the exceptions in suppressed. Somehow in ingestion flow, we are getting the Value in coerce as new byte[0] with SqlType BIGINT which is throwing the following exceptions.
Error: Unknown exception Cannot coerce [[B] to BIGINT

People had hard time in finding out which field is causing the exception while querying. Thus, this change is logging the fieldName along with exception. This will help us identify the issues in the ingestion and we can figure why are we ingesting primitive arrays.

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Release note

Key changed/added classes in this PR

MyFoo
OurBar
TheirBaz

This PR has:

LakshSingla

Thanks for the PR!

Can you add a SQL test (CalciteQueryTest) and/or an MSQ query that shows the coercion error

LakshSingla · 2023-06-24T02:40:59Z

+  public void testCoerceOfArrayOfPrimitives()
+  {
+    try {
+      ObjectMapper mapper = new ObjectMapper();


nit: I think we should initialize this outside the test case since it would assist any other tests trying to use an object mapper.

Suggested change

ObjectMapper mapper = new ObjectMapper();

ObjectMapper mapper = new DefaultObjectMapper();

While it might not affect this use case, DefaultObjectMapper() is preferred since it contains Druid's modules having custom serdes.

LakshSingla · 2023-06-24T02:45:05Z

+  private static String prepareCoerceException(Object value, SqlTypeName sqlTypeName, String fieldName)
+  {
+    return StringUtils.format(
+        "Cannot coerce field [%s] of value with class [%s] to %s",


Suggested change

"Cannot coerce field [%s] of value with class [%s] to %s",

"Cannot coerce field [%s] of value with class [%s] to [%s]",

"Cannot coerce field [%s] of type [%s] to type [%s]"

LakshSingla · 2023-06-24T02:47:37Z

+
+  private static ISE coerceExceptionWithSupressed(Object value, SqlTypeName sqlTypeName, String fieldName, Throwable t)
+  {
+    ISE e = new ISE(prepareCoerceException(value, sqlTypeName, fieldName));


We should use the newly added DruidException class since it's a using-facing message.

LakshSingla · 2023-06-24T02:58:34Z

+  private static ISE coerceExceptionWithSupressed(Object value, SqlTypeName sqlTypeName, String fieldName, Throwable t)
+  {
+    ISE e = new ISE(prepareCoerceException(value, sqlTypeName, fieldName));
+    e.addSuppressed(t);


I am not very clear with the semantics of adding t as suppressed v/s adding it as a cause.
Since t is directly causing the coercion exception that we are raising, I suppose it should be a cause. right?

added the new DruidExceptions

abhishekagarwal87 · 2023-06-24T06:25:19Z

+      Assert.fail("Should throw an exception");
+    }
+    catch (Exception e) {
+      Assert.assertEquals("Cannot coerce field [testFieldName] of value with class [[B] to BIGINT", e.getMessage());


can we also more friendly type names such as binary instead of [[B?

We could add the mapping of primitive array class names to actual names. Following list is standard to Strings for primitive java arrays classnames.
[Z = boolean
[B = byte
[S = short
[I = int
[J = long
[F = float
[D = double
[C = char
[L = any non-primitives(Object)

pranavbhole · 2023-06-28T00:25:06Z

Thanks for the PR!

Can you add a SQL test (CalciteQueryTest) and/or an MSQ query that shows the coercion error

Coerce error happens while indexing the hll sketch columns with empty values. test framework was not ingesting it as correct long values and I was not able to add test that has byte array in the ingestions.

pranavbhole · 2023-06-28T00:25:11Z

@LakshSingla @abhishekagarwal87 Can you please review?

abhishekagarwal87 · 2023-06-29T06:55:01Z

+      Assert.fail("Should throw an exception");
+    }
+    catch (Exception e) {
+      Assert.assertEquals("Cannot coerce field [fieldName] class of [[B] to [BIGINT]", e.getMessage());


weren't we going to replace [[B] with a more friendly name such as binary?

added mapping

abhishekagarwal87 · 2023-06-29T06:55:14Z

+                             t,
+                             "Cannot coerce field [%s] class of [%s] to [%s]",
+                             fieldName,
+                             value == null ? "null" : value.getClass().getName(),


Suggested change

value == null ? "null" : value.getClass().getName(),

value == null ? "unknown" : value.getClass().getName(),

abhishekagarwal87 · 2023-06-29T06:55:57Z

  )
  {
-    return new ISE(t, "Cannot coerce [%s] to [%s]", value == null ? "null" : value.getClass().getName(), sqlTypeName);
+    return DruidException.forPersona(DruidException.Persona.DEVELOPER)


Hmm. I would say that this is for user persona since its an invalid input.

abhishekagarwal87 · 2023-06-29T06:56:48Z

+                         .ofCategory(DruidException.Category.INVALID_INPUT)
+                         .build(
+                             t,
+                             "Cannot coerce field [%s] class of [%s] to [%s]",


How about including an action message here? @clintropolis - any suggestions?

pranavbhole · 2023-06-30T01:40:24Z

@abhishekagarwal87 @clintropolis please review

…java fix the logging Co-authored-by: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com>

Logging the fieldName in the coerce exceptions

LakshSingla reviewed Jun 24, 2023

View reviewed changes

abhishekagarwal87 reviewed Jun 24, 2023

View reviewed changes

pranavbhole force-pushed the improvedLoggingIn_Coerce branch 2 times, most recently from f9a6840 to ec7687c Compare June 28, 2023 00:18

clintropolis added Area - SQL Ease of Use labels Jun 28, 2023

pranavbhole force-pushed the improvedLoggingIn_Coerce branch 2 times, most recently from 5f6cc9b to ea6a82c Compare June 28, 2023 21:34

abhishekagarwal87 reviewed Jun 29, 2023

View reviewed changes

pranavbhole force-pushed the improvedLoggingIn_Coerce branch from ea6a82c to c99dd83 Compare June 30, 2023 01:25

pranavbhole force-pushed the improvedLoggingIn_Coerce branch from c99dd83 to f0ad7b6 Compare June 30, 2023 02:39

Sync up with master

dc38717

pranavbhole force-pushed the improvedLoggingIn_Coerce branch from f0ad7b6 to dc38717 Compare June 30, 2023 03:04

abhishekagarwal87 reviewed Jun 30, 2023

View reviewed changes

Comment thread sql/src/main/java/org/apache/druid/sql/calcite/run/SqlResults.java Outdated

pranavbhole force-pushed the improvedLoggingIn_Coerce branch from 9d6e95f to 02ed241 Compare June 30, 2023 20:34

Update sql/src/main/java/org/apache/druid/sql/calcite/run/SqlResults.…

4624644

…java fix the logging Co-authored-by: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com>

pranavbhole force-pushed the improvedLoggingIn_Coerce branch from 02ed241 to 4624644 Compare June 30, 2023 22:00

abhishekagarwal87 approved these changes Jul 3, 2023

View reviewed changes

abhishekagarwal87 merged commit 2d5b273 into apache:master Jul 3, 2023

abhishekagarwal87 added this to the 27.0 milestone Jul 19, 2023

sergioferragut pushed a commit to sergioferragut/druid that referenced this pull request Jul 21, 2023

Logging the fieldName in the coerce exceptions (apache#14483)

dfb2f2c

Logging the fieldName in the coerce exceptions

	ObjectMapper mapper = new ObjectMapper();
	ObjectMapper mapper = new DefaultObjectMapper();

	"Cannot coerce field [%s] of value with class [%s] to %s",
	"Cannot coerce field [%s] of value with class [%s] to [%s]",

	value == null ? "null" : value.getClass().getName(),
	value == null ? "unknown" : value.getClass().getName(),

Conversation

pranavbhole commented Jun 24, 2023 • edited by abhishekagarwal87 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Release note

Key changed/added classes in this PR

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!

zachjsh Jun 24, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavbhole commented Jun 28, 2023

Uh oh!

pranavbhole commented Jun 28, 2023

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!

pranavbhole commented Jun 30, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pranavbhole commented Jun 24, 2023 •

edited by abhishekagarwal87

Loading

zachjsh Jun 24, 2023 •

edited

Loading