Frame writers: Coerce numeric and array types in certain cases. by gianm · Pull Request #16994 · apache/druid

gianm · 2024-09-03T19:31:05Z

This patch adds TypeCastSelectors, which is used when writing frames to perform two coercions:

When a numeric type is desired and the underlying type is non-numeric or unknown, the underlying selector is wrapped, getObject is called and the result is coerced using ExprEval.ofType. This differs from the prior behavior where the primitive methods like getLong, getDouble, etc, would be called directly. This fixes an issue where a column would be read as all-zeroes when its SQL type is numeric and its physical type is string, which can happen when evolving a column's type from string to number.
When an array type is desired, the underlying selector is wrapped, getObject is called, and the result is coerced to Object[]. This coercion replaces some earlier logic from Numeric array support for columnar frames #15917.

This mirrors similar logic in numeric aggregators. (The same method from AggregatorUtil is even used to determine when to apply the logic.) The idea is that when an underlying selector is STRING or COMPLEX typed, we should call getObject and cast the result to number, rather than using the primitive numeric accessor methods. This fixes an issue where a column would be read as all-zeroes when its SQL type is numeric, and its physical type is string. This can happen when evolving a column's type from string to number.

jon-wei · 2024-09-03T20:59:10Z

+  public double getDouble()
+  {
+    final Number n = computeIfNeeded();
+    return n == null ? NullHandling.ZERO_DOUBLE : n.floatValue();


should this call n.doubleValue()?

I figured since this is the "to float" selector, it should use float precision even if the caller is requesting a double.

Fwiw, the caller really shouldn't be requesting a double anyway (since it's a "to float" selector). I have tests for this method but I don't expect it to be called.

cryptoe · 2024-09-04T05:21:10Z

+      return ((Number) obj).doubleValue();
+    } else {
+      final ExprEval<?> eval = ExprEval.bestEffortOf(obj);
+      return eval.isNumericNull() ? null : eval.asDouble();


Should we through a parse exception here ?

Going though the exprEval code, it seems that if a string expression eval is returned and its not a valid double, then a null is returned.
Shoudn't we throw an exception and force the user to add a cast to is sql statement ?

After discussion with @clintropolis , the automatic type conversion is used all over the processing stack and the ingestion stack. Hence, parseException may not be correct.

Yeah, this sort of coercion behavior is standard in the query stack when reading from selectors that don't match the desired type. It's how we accomplish schema evolution.

clintropolis · 2024-09-04T09:07:26Z

+
+      switch (desiredType.getType()) {
+        case LONG:
+          return new ObjectToLongColumnValueSelector(selector, rowIdSupplier);


it might be a little bit nicer to get the column capabilities from the selector factory so we know the 'input' column type and pass that into these ObjectToType wrapper selectors so that they could use ExprEval.ofType instead of ExprEval.bestEffortOf which should be a bit more efficient since the latter is mostly these days reserved for uses where we do not know the input type

That makes sense, I'll change it.

…too.

gianm · 2024-09-04T16:51:13Z

Updated PR to:

Use ExprEval.ofType rather than bestEffortOf
Move array typecasting logic into TypeCastSelectors, instead of the field writers.
Update an IT that was failing because a double_col: 0 turned into a double_col: null when double_col was actually stored as STRING with value extra. This is expected.

clintropolis · 2024-09-04T17:06:43Z

+  {
+    final Object obj = selector.getObject();
+    if (obj == null) {
+      return ExprEval.of(null);


nit: could use ExprEval.ofType(selectorType, null) so it doesn't become a ExpressionType.STRING, but also probably doesn't matter

It still does become a string if we do that, b/c of how the code flow works through ExprEval.ofType. Maybe it shouldn't? But it does.

if type is null it becomes a string, but if the value is null it is an ExprEval of that type with a null value

the object from the row adapter.

gianm · 2024-09-04T22:14:50Z

I had to expand the scope of this patch to fix a failing test case in FrameBasedInlineDataSourceSerializerTest. This part is added:

This patch also fixes up selectors RowBasedColumnSelectorFactory#makeColumnValueSelector to adhere to the contract of BaseObjectColumnValueSelector#getObject. In particular, the column selector factory now enforces the requirement that Number is returned for columns of numeric type, and Object[] is returned for selectors of array type. This is connected to the above change, because frame writers now rely on this contract being adhered to.

…Selectors.

gianm · 2024-09-05T06:00:58Z

I just pushed a new approach that no longer modifies RowBasedColumnSelectorFactory. The blast radius of that change was a little more than I bargained for. Instead, I updated TypeCastSelectors to apply the array coercion always, even if the underlying selector claims to offer the same array type that we want. I do think it'd be good to update RowBasedColumnSelectorFactory at some point though, since it's currently used in ways that violate the BaseObjectColumnValueSelector#getObject contract. (in particular, when the ColumnInspector declares type ARRAY<X> for a column, but the RowAdapter does not provide Object[].)

When merging this patch, please use the PR title + description for the commit message. Letting GitHub use the list of commits for the message is going to be too confusing, since the approach changed a bunch of times.

clintropolis · 2024-09-05T10:40:32Z

+    } else if (type == ValueType.ARRAY) {
+      final TypeSignature<ValueType> elementType = desiredType.getElementType();
+
+      if (obj instanceof List) {


you could use ExprEval.bestEffortArray here i suppose, though ExprEval.ofType if desiredType is an array, as well as ExprEval.bestEffortOf should have pretty similar logic.

See #16994 (comment), I pushed a new commit that replaces this method with ExprEval.ofType.

clintropolis · 2024-09-05T10:43:44Z

+   * @param desiredType desired type
+   */
+  @Nullable
+  public static Object bestEffortCoerce(


this method seems odd to me, it is basicaly like ExprEval.ofType which separates handling based on the expected ExpressionType (instead of ColumnType like is used here), but then uses ExprEval.bestEffortOf inside of the cases (which uses a much larger set of instanceof checks than ExprEval.ofType).

What sets this method apart from the others?

I pushed a new commit that replaces this method with ExprEval.ofType. It seems to do what we need. I didn't try it at first, because I misunderstood what it does— I didn't realize it also included coercion. I added javadoc to ExprEval.ofType explaining that it does do coercion.

clintropolis · 2024-09-05T18:01:03Z

+  /**
+   * Create an eval of the provided type. Coerces the provided object to the desired type.
+   *
+   * @param type  type, or null to be equivalent to {@link #bestEffortOf(Object)}
+   * @param value object to be coerced to the type
+   */


gianm added Bug Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Sep 3, 2024

LakshSingla reviewed Sep 3, 2024

View reviewed changes

Comment thread processing/src/main/java/org/apache/druid/frame/field/FieldWriters.java

jon-wei reviewed Sep 3, 2024

View reviewed changes

Merge branch 'master' into frame-typecast-selector

6638d87

cryptoe reviewed Sep 4, 2024

View reviewed changes

clintropolis reviewed Sep 4, 2024

View reviewed changes

Merge branch 'master' into frame-typecast-selector

d374fe1

gianm changed the title ~~Frame writers: use getObject when reading string/complex types.~~ Frame writers: cast when reading numbers/arrays from selectors of different underlying type. Sep 4, 2024

jon-wei approved these changes Sep 4, 2024

View reviewed changes

Use ExprEval.ofType rather than bestEffortOf. Add casting for arrays …

f3aea17

…too.

gianm requested review from clintropolis and jon-wei September 4, 2024 16:51

clintropolis reviewed Sep 4, 2024

View reviewed changes

clintropolis approved these changes Sep 4, 2024

View reviewed changes

Coerce in RowBasedColumnSelectorFactory when desired type doesn't match

e224671

the object from the row adapter.

github-actions Bot added the Area - Segment Format and Ser/De label Sep 4, 2024

Fix error.

5f58b59

gianm changed the title ~~Frame writers: cast when reading numbers/arrays from selectors of different underlying type.~~ Additional object coercion in FrameWriters, RowBasedColumnSelectorFactory. Sep 4, 2024

clintropolis approved these changes Sep 5, 2024

View reviewed changes

Undo RowBasedColumnSelectorFactory change. Account for it in TypeCast…

f1985d8

…Selectors.

github-actions Bot added the Area - Batch Ingestion label Sep 5, 2024

gianm changed the title ~~Additional object coercion in FrameWriters, RowBasedColumnSelectorFactory.~~ FrameWriters: Coerce numeric and array types in certain cases. Sep 5, 2024

gianm changed the title ~~FrameWriters: Coerce numeric and array types in certain cases.~~ Frame writers: Coerce numeric and array types in certain cases. Sep 5, 2024

clintropolis reviewed Sep 5, 2024

View reviewed changes

Use ExprEval#ofType.

c0522e5

Javadoc.

28d36e1

clintropolis approved these changes Sep 5, 2024

View reviewed changes

gianm merged commit 175636b into apache:master Sep 6, 2024

gianm deleted the frame-typecast-selector branch September 6, 2024 00:20

gianm mentioned this pull request Sep 24, 2024

various fixes and improvements to vectorization fallback #17098

Merged

kfaraz added this to the 31.0.0 milestone Oct 4, 2024

Conversation

gianm commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptoe Sep 4, 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!

gianm commented Sep 4, 2024

Uh oh!

clintropolis Sep 4, 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!

gianm commented Sep 4, 2024

Uh oh!

gianm commented Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gianm commented Sep 3, 2024 •

edited

Loading

cryptoe Sep 4, 2024 •

edited

Loading

clintropolis Sep 4, 2024 •

edited

Loading

gianm commented Sep 5, 2024 •

edited

Loading