Deserialize complex dimensions in group by queries to their respective types when reading from spilled files and cached results by LakshSingla · Pull Request #16620 · apache/druid

LakshSingla · 2024-06-17T17:41:53Z

Description

Like #16511, but for keys that have been spilled or cached during the grouping process. This was not caught in the original PR since there aren't tests for it. This PR adds the necessary deserialization, as well as the tests that fail if this patch and #16511 aren't in.

This PR has:

clintropolis · 2024-06-21T00:21:15Z

+            if (includeTimestamp && dimsReadSoFar == 0) {
+              // Read the timestamp
+              objects[dimsReadSoFar] = codec.readValue(jp, Long.class);
+            } else {


it might be nice to pull this out of the while loop?

if (includeTimestamp && jp.currentToken() != JsonToken.END_ARRAY) { objects[dimsReadSoFar++] = codec.readValue(jp, Long.class); jp.nextToken(); } while (jp.currentToken() != JsonToken.END_ARRAY) { if (dimsReadSoFar >= dimsToRead) { throw DruidException.defensive("More dimensions encountered than expected [%d]", dimsToRead); } DruidException.conditionalDefensive( dimsReadSoFar < dimsToRead, "Insufficient serde helpers present" ); // Read the dimension if (serdeHelpers[dimsReadSoFar - timestampAdjustment].getComplexClazz() == null) { objects[dimsReadSoFar] = codec.readValue(jp, Object.class); if (objects[dimsReadSoFar] instanceof Integer) { objects[dimsReadSoFar] = ((Integer) objects[dimsReadSoFar]).longValue(); } else if (objects[dimsReadSoFar] instanceof Double) { objects[dimsReadSoFar] = ((Double) objects[dimsReadSoFar]).floatValue(); } } else { objects[dimsReadSoFar] = codec.readValue(jp, serdeHelpers[dimsReadSoFar - timestampAdjustment].getComplexClazz()); } ++dimsReadSoFar; jp.nextToken(); }

I tried this, but the complexity is the same, given that we'd have to duplicate the defensive checks, loop counter increment and json stream increment. I have left it as is for now.

i guess my thought was that we would be removing one conditional check of the loop of every column of every row which could add up, but i haven't really measured one way or another. This is a pretty hot loop though so worth thinking about making it as efficient as possible

I was looking at it from code's complexity perspective. Your approach is better to make it more efficient. I'd guess most of the time branch predictions would make it quite equivalent. I'll make the changes to make it explicit however.

clintropolis · 2024-06-21T00:22:17Z

+                if (objects[dimsReadSoFar] instanceof Integer) {
+                  objects[dimsReadSoFar] = ((Integer) objects[dimsReadSoFar]).longValue();
+                } else if (objects[dimsReadSoFar] instanceof Double) {
+                  objects[dimsReadSoFar] = ((Double) objects[dimsReadSoFar]).floatValue();
+                }


could serde helpers just tell this to use Float.class and Long.class instead of generically doing Object.class and then checking it? (e.g. make key serde helpers return non-null classes instead of only complex types being non-null)

I have modified this. I had a doubt about the original flow. In the case of Double dimensions, we would deserialize them as double and cast them as float right, which seems incorrect. Modified PR seems correct, but it differs from the original behavior.

clintropolis · 2024-06-21T00:24:12Z

+   * work correctly when deserialized as generic java objects without type information.
+   */
+  @Nullable
+  Class<?> getComplexClazz();


i suppose this can just be called getComplexClass (the reason for getClazz is to not conflict with getClass), unless we do the other suggestion and make it not nullable and just use it for all the json serde, in which case it should be getClazz or getJsonClass or something

clintropolis · 2024-07-03T09:04:03Z

  public abstract TypeReference<ResultType> getResultTypeReference();

+  /**
+   *


nit: please add javadocs or remove (but preferably add and what does null mean if it returns null like the other one has).

Also, should this be deprecated?

clintropolis · 2024-07-03T09:05:27Z

+  @Override
+  public CacheStrategy getCacheStrategy(DataSourceMetadataQuery query, @Nullable ObjectMapper mapper)
+  {
+    return null;


nit: if this is returning null is the override really needed?

added for clarity, will remove it.

ah maybe is safer to leave since default is to not cache I guess? I don't feel super strongly either way

clintropolis · 2024-07-03T09:10:08Z

+        }
+        classes[i] = nullableTypeStrategy.getClazz();
+      } else {
+        classes[i] = Object.class;


any reason not to use the real class here?

clintropolis · 2024-07-03T09:13:51Z

+            if (includeTimestamp && dimsReadSoFar == 0) {
+              // Read the timestamp
+              objects[dimsReadSoFar] = codec.readValue(jp, Long.class);
+            } else {


i guess my thought was that we would be removing one conditional check of the loop of every column of every row which could add up, but i haven't really measured one way or another. This is a pretty hot loop though so worth thinking about making it as efficient as possible

clintropolis · 2024-07-03T09:15:27Z

+      }
+
+      final ObjectMapper newObjectMapper = spillMapper.copy();
+      newObjectMapper.registerModule(new SpillModule());


should this be a static somewhere so we don't have to make a new one all the time?

The new deserialization method is linked to a particular query. That is why we need to create one for each query.

oh right, i can't read 😜 , sorry

clintropolis · 2024-07-03T09:17:46Z

+      @Override
+      public Class<?> getClazz()
+      {
+        return Object.class;


same thing Object[].class

clintropolis · 2024-07-03T09:18:25Z

+      @Override
+      public Class<?> getClazz()
+      {
+        return Object.class;


String.class?

Similar explanation as to why I left it as Object.class to preserve existing behavior rather than being clearer and mentioning String.class.
I doubt we'd run into regressions with the latter, but I was being overtly cautious.
wdyt?

clintropolis · 2024-07-03T09:20:01Z

+        if (columnType.is(ValueType.COMPLEX)) {
+          clazz = columnType.getNullableStrategy().getClazz();
+        } else {
+          clazz = Object.class;


why not always use the column type?

clintropolis · 2024-07-03T09:22:32Z

  {
    NullHandling.initializeForTests();
+    //noinspection ResultOfObjectAllocationIgnored
+    new AggregatorsModule();


hmm, we should move the complex serde registrations of that module to a static method it calls instead of doing this so its more obvious what is going on here

cool, ill do that. It's weird that we need to call this in the first place, I would have expected the stuff to be registered. I'll make it more explicit and add a comment.

nit: forget about this?

oops, yeah. Will fix it up

clintropolis · 2024-07-03T09:27:47Z

-                  DimensionHandlerUtils.convertObjectToType(results.next(), dimensionSpec.getOutputType())
-              );
-
+              if (dimensionSpec.getOutputType().is(ValueType.COMPLEX)) {


i kind of wonder if we should special handle json (COMPLEX<json>) here since it is kind of a special complex type (that really should probably be a built-in standard type instead of complex...) because convertValue looks pretty expensive.

because convertValue looks pretty expensive.

That sounds reasonable.
Maybe as an alternative, what if we return Object.class instead of StructuredValue.class for jsonStrategy.getClazz()? I assume that convertValue should short-circuit in that case - converting it from an object to object. If that's the case, we'd get away with special handling (perhaps).

+            );
+
+            // Read the dimension
+            serdeHelpers[dimsReadSoFar - timestampAdjustment].getClazz();


+            // Read the dimension
+            serdeHelpers[dimsReadSoFar - timestampAdjustment].getClazz();
+            objects[dimsReadSoFar] =
+                codec.readValue(jp, serdeHelpers[dimsReadSoFar - timestampAdjustment].getClazz());


LakshSingla · 2024-07-08T05:19:28Z

+  public <T> CacheStrategy<ResultType, T, QueryType> getCacheStrategy(QueryType query, @Nullable ObjectMapper mapper)
  {
-    return null;
+    return getCacheStrategy(query);


Not relevant, the deprecated method is used to maintain backward compatibility.

This reverts commit c2724b9.

clintropolis · 2024-07-10T06:14:33Z

+                  DruidException.conditionalDefensive(
+                      mapper != null,
+                      "Cannot deserialize complex dimension from if object mapper is not provided"
+                  );


nit: i wonder if this should be outside of the loop or detected outside of it at least before we get here? like maybe while doing createDimensionClasses or something

Makes sense, I'll move the checks there.

clintropolis · 2024-07-10T06:16:29Z

+      }
+
+      final ObjectMapper newObjectMapper = spillMapper.copy();
+      newObjectMapper.registerModule(new SpillModule());


oh right, i can't read 😜 , sorry

clintropolis · 2024-07-10T06:17:24Z

+      @Override
+      public Class<?> getClazz()
+      {
+        return Object.class;


i guess i was wondering if maybe could help get rid of some of the coerce list to array stuff, but totally fine to explore this later

clintropolis · 2024-07-10T06:18:05Z

  {
    NullHandling.initializeForTests();
+    //noinspection ResultOfObjectAllocationIgnored
+    new AggregatorsModule();


nit: forget about this?

cryptoe · 2024-07-10T12:47:18Z

+
+          while (jp.currentToken() != JsonToken.END_ARRAY) {
+
+            DruidException.conditionalDefensive(


Can we remove these checks because they are running per row ? Is it worth having this overhead in the hot path ?

…e types when reading from spilled files and cached results (apache#16620) Like apache#16511, but for keys that have been spilled or cached during the grouping process

init

57e8512

github-advanced-security AI found potential problems Jun 17, 2024

View reviewed changes

LakshSingla added 4 commits June 17, 2024 23:49

javadocs

12f1d08

codeql

7fd9fa8

test rename, codeql

f1fc861

take timestamps into account

5590ad6

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

github-advanced-security AI found potential problems Jun 18, 2024

View reviewed changes

Comment thread ...essing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java Fixed

Comment thread ...essing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java Fixed

LakshSingla added 3 commits June 18, 2024 13:59

generic

94b38a8

Merge branch 'master' into spill-deserialize

9e36e42

codeql, add defensive check

f647c3f

clintropolis reviewed Jun 21, 2024

View reviewed changes

review

1c8561c

github-advanced-security AI found potential problems Jun 24, 2024

View reviewed changes

Comment thread ...essing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java Fixed

Comment thread ...essing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java Fixed

LakshSingla added 3 commits June 24, 2024 22:31

array numeric left

3dc54d3

test changes for caching

33d55ad

modifications

c57c5ff

LakshSingla changed the title ~~Deserialize complex dimensions in group by queries to their respective types when reading from spilled files~~ Deserialize complex dimensions in group by queries to their respective types when reading from spilled files and cached results Jun 26, 2024

clintropolis reviewed Jul 3, 2024

View reviewed changes

review

1f3ff61

github-advanced-security AI found potential problems Jul 8, 2024

View reviewed changes

LakshSingla added 2 commits July 8, 2024 10:52

maybe codeql

c2724b9

Revert "maybe codeql"

e9eec6a

This reverts commit c2724b9.

clintropolis approved these changes Jul 10, 2024

View reviewed changes

cryptoe reviewed Jul 10, 2024

View reviewed changes

LakshSingla added 3 commits July 12, 2024 10:23

reviews

6f868d1

codeql

abd8274

faster loop

c0594cf

LakshSingla merged commit 209f8a9 into apache:master Jul 15, 2024

LakshSingla deleted the spill-deserialize branch July 15, 2024 09:30

kfaraz added this to the 31.0.0 milestone Oct 4, 2024


		while (jp.currentToken() != JsonToken.END_ARRAY) {

		DruidException.conditionalDefensive(

Conversation

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

Uh oh!

Description

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

LakshSingla Jun 24, 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!

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!

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!

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 failure

Check failure

Check notice

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LakshSingla commented Jun 17, 2024 •

edited

Loading

LakshSingla Jun 24, 2024 •

edited

Loading