Refactor SemanticCreator by adarshsanjeev · Pull Request #16700 · apache/druid

adarshsanjeev · 2024-07-08T10:09:56Z

Refactors the SemanticCreator annotation.

Moves the interface to the semantic package.
Create a SemanticUtils to hold logic for storing semantic maps.
Add FrameMaker interface.

This PR has:

kgyrtkirk · 2024-07-08T11:04:43Z

+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.segment.column.RowSignature;
+
+public interface FrameMaker


I think its not necessary needed to introduce an interface if there will be only 1 implementation

I wonder why not somehow make RowsAndColumns.as(Frame.class) and
RowsAndColumns.as(RowSignature.class) work for this?

While this currently only has toColumnBasedFrame(), it's equally plausible to have a toRowBasedFrame() and allow implementations to ask for the one that they expect to be working with. The notion of which kind of Frame that you are getting is not something that can be relayed with RowsAndColumns.as(Frame.class).

That said, one thing that does annoy me about the .as() and the semantic interfaces is that you have these factory-style interfaces for when you are building a thing. I don't know of another way to deal with it and maintain the extensibility, so I've just been thinking that it's the price we pay.

currently the DefaultFrameMaker class has no internal state - and with these methods I think there won't be any - unless it tries to cache the frame or something...

I think one of the key problems with Frame is that its decoupled from RowSignature - however most of the time those two should be together....why not introduce a class which could hold a Frame along with the RowSignature or extend frame somehow to optionally add a RowSignature
that way a single as method could be implemented; and there won't be a need for the FrameMaker

my recent experiences sugest that the production of a frame from a rac may not need to be aligned as 1-1 with it; so I think a class to which rac-s would be written to produce a frame would be more like what we will need in the future - other way to get to to more-or-less the same is to use ConcatRowAndColumns

would it be possible to use the approach you end up with at the places where we have this frame creation logic copied-to already?

LazilyDecoratedRowsAndColumns (2 places)

StorageAdapterRowsAndColumns

I like this idea, aligning a RowSignature with a Frame is definitely a really annoying thing when dealing with Frames, having the Frame carry it forward is nice. That said, one of the reasons for this interface is, if we assume that a Frame is the serialization format that we want for the wire, then we will want some "standard" means of converting from RowsAndColumns to the concrete wire-serializable form.

kgyrtkirk · 2024-07-08T11:11:47Z

+
+public class SemanticUtils
+{
+  private static final Map<Class<?>, Map<Class<?>, Function<?, ?>>> OVERRIDES = new LinkedHashMap<>();


why do we need to put everything into a centralized map?
there is no point in caching this as all filed which are calling makeAsMap are static fields for which only once that method will be invoked

This exists to allow extensions to register new interfaces and implementations without needing to impact the core code. This is probably worthy of javadoc.

Added a javadoc

you mean to override default behaviour?
could you give an example? how this supposed to work if 2-3 classes want to override the same?

what's the problem you are trying to solve?

Essentially, we could have a class A that has a SemanticCreator toB(). This allows A.as(B.class) to work.

However, there could be extensions that want a different implementation to be bound. The extension would want to use that implementation only if the extension is loaded and also, it would not be able to change the first binding. Overrides will allow the extension to bind something like:

SemanticUtils.registerAsOverride( A.class, B.class, (a) -> new C(a) // C extends B );

to modify
If there are multiple bindings for A and B from different places, SemanticUtils would throw an exception.

kgyrtkirk · 2024-07-08T11:22:52Z

-  @Nullable
-  private final Metadata metadata;
+  @NotNull
+  private final Supplier<Metadata> metadataSupplier;


this looks odd - an index with named columns ; but the Metadata is softened with a supplier
why soften Metadata with a Supplier ?

I'm not 100% certain that this is why it was done, but one benefit of softening it could be to avoid holding the object in memory which potentially softens memory pressure. There's probably a legitimate question to be asked if this should really be a Supplier<> or if SimpleQueryableIndex should be made an abstract class.

Converted it to an abstract class

imply-cheddar

Overall LGTM

imply-cheddar · 2024-07-09T04:58:05Z

+
+public class SemanticUtils
+{
+  private static final Map<Class<?>, Map<Class<?>, Function<?, ?>>> OVERRIDES = new LinkedHashMap<>();


This exists to allow extensions to register new interfaces and implementations without needing to impact the core code. This is probably worthy of javadoc.

imply-cheddar · 2024-07-09T05:02:38Z

+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.segment.column.RowSignature;
+
+public interface FrameMaker


While this currently only has toColumnBasedFrame(), it's equally plausible to have a toRowBasedFrame() and allow implementations to ask for the one that they expect to be working with. The notion of which kind of Frame that you are getting is not something that can be relayed with RowsAndColumns.as(Frame.class).

That said, one thing that does annoy me about the .as() and the semantic interfaces is that you have these factory-style interfaces for when you are building a thing. I don't know of another way to deal with it and maintain the extensibility, so I've just been thinking that it's the price we pay.

imply-cheddar · 2024-07-09T05:06:14Z

-  @Nullable
-  private final Metadata metadata;
+  @NotNull
+  private final Supplier<Metadata> metadataSupplier;


I'm not 100% certain that this is why it was done, but one benefit of softening it could be to avoid holding the object in memory which potentially softens memory pressure. There's probably a legitimate question to be asked if this should really be a Supplier<> or if SimpleQueryableIndex should be made an abstract class.

imply-cheddar · 2024-07-09T05:15:39Z

Looking at the failures, this needs improved coverage. For SemanticUtils, it should be fine to create a class like inside of SemanticUtilsTest.ClassForSemanticUtilsCoverage that you then register implementations against and run things. This should allow you to validate capabilities without the static nature of the internal object becoming an issue.

+    }
+
+    @SemanticCreator
+    public AInterface toAInterface(String invalidParameter)


kgyrtkirk · 2024-07-22T07:31:14Z

 */
 public interface ColumnarInts extends IndexedInts, Closeable
 {
+  default void get(int[] out, int offset, int start, int length)


are there any benefit of implementing this method here - and not in IndexedInts which already has a get method?

kgyrtkirk · 2024-07-22T07:32:30Z

        }

        final int limit = Math.min(length - p, sizePer - bufferIndex);
-        reader.read(out, p, bufferIndex, limit);


are there any tests which are passing not 0 as offset?

kgyrtkirk · 2024-07-22T07:40:09Z

+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.segment.column.RowSignature;
+
+public interface FrameMaker


currently the DefaultFrameMaker class has no internal state - and with these methods I think there won't be any - unless it tries to cache the frame or something...

I think one of the key problems with Frame is that its decoupled from RowSignature - however most of the time those two should be together....why not introduce a class which could hold a Frame along with the RowSignature or extend frame somehow to optionally add a RowSignature
that way a single as method could be implemented; and there won't be a need for the FrameMaker

my recent experiences sugest that the production of a frame from a rac may not need to be aligned as 1-1 with it; so I think a class to which rac-s would be written to produce a frame would be more like what we will need in the future - other way to get to to more-or-less the same is to use ConcatRowAndColumns

would it be possible to use the approach you end up with at the places where we have this frame creation logic copied-to already?

LazilyDecoratedRowsAndColumns (2 places)

StorageAdapterRowsAndColumns

kgyrtkirk · 2024-07-22T07:42:43Z

+          @Override
+          public Metadata getMetadata()
+          {
+            return null;


if its valid to have null metadata; it could be the default implementation in the interface

kgyrtkirk · 2024-07-22T07:47:09Z

+  @Nullable
+  default <T> T as(Class<? extends T> clazz)


since this PR is about to make the SemanticCreator more mainstream: could you please introduce an interface for it and use it everywhere ?

I didn't understand this, do you mean adding a new interface like "SemanticClass" that all other classes would implement if it has a SemanticCreator?

yes - exactly

kgyrtkirk · 2024-07-22T07:52:36Z

+import java.util.Map;
+import java.util.function.Function;
+
+public class SemanticUtils


this Utils could be renamed to be the interface containing the as for these things; the static method could remain as those are providing services...

please also add apidoc how this as stuff works/etc

I like the idea about creating the interface for .as() and having these utils be static helpers on it. As a refactor, that should probably be done for all of the places that are using an .as() so I think I'd like to let this current PR go through (as it's following the current pattern in the code) and one of us can take on the refactor as you defined it as I do think that's an improvement.

kgyrtkirk · 2024-07-22T07:54:46Z

+  @Nullable
+  public <T> T as(Class<? extends T> clazz)
+  {
+    return column.as(clazz);


this is a non-overridable as method....or not?

how will I be able to override this with SemanticUtils.registerOverride?

The class does not use SemanticUtils.makeAsMap so it would not be directly overridable. I think the decision of using overrides or not currently belongs to the class itself, in this way. The extension can still override it by adding an override for ColumnarLongs which this class is pointing to.

kgyrtkirk · 2024-07-22T07:55:33Z

+  @Nullable
+  default <T> T as(Class<? extends T> clazz)
+  {
+    return null;


this is a non-overridable as method....or not?
I think registerOverride should also work when it was not provided before....

how will I be able to override this with SemanticUtils.registerOverride?

The intention is to have implementations of base column be classes that have semantic creators and have possible conversions. Overrides can be registered against them.
If a class does not have any conversion to another class, and does not override this function, it defaults to null.

Adding a map and allowing functions to register overrides to it may cause it to use the conversions from the base class, if it is not overridden. I am not sure if that behavior is expected.

kgyrtkirk · 2024-07-22T07:57:34Z

+    get(out, 0, start, length);
+  }
+
+  default void get(long[] out, int offset, int start, int length)


what's driving this change?
why the need for this here and not in other classes like ColumnarDoubles ?

kgyrtkirk · 2024-07-22T08:01:50Z

+        public Metadata getMetadata()
+        {
+          try {
+            ByteBuffer metadataBB = smooshedFiles.mapFile("metadata.drd");


I doesn't really know these parts; but this will mean that the Metadata will be read from disk every time its asked for; is that desired?

Not necessarily. The smooshedFiles is memory mapped and this is returning a memoized memory-mapped buffer. It will do some less than fast operations, but not necessarily brand new disk IO. The getMetadata() call is used seldom (only a few specific introspection-oriented calls like the segmentMetadata query) so it's better to have it be lazy and not consume on-heap memory.

cheddar · 2024-08-06T16:29:31Z

I had some off-line conversations with Zoltan about his comments. He's got some great comments for cleaning things up. I like eliminating SemanticUtils and just putting it on the interface that we use for the .as() method. And I believe that having the RowSignature alongside the Frame is a good thing. We will allow them to be done in future PRs, so I'm going to merge this as is.

adarshsanjeev added 2 commits July 8, 2024 15:37

Refactor SemanticCreator

82c10e8

Refactor SimpleQueryableIndex to take a metadata supplier.

15b2bb6

github-actions Bot added Area - Segment Format and Ser/De Area - Ingestion labels Jul 8, 2024

kgyrtkirk reviewed Jul 8, 2024

View reviewed changes

imply-cheddar approved these changes Jul 9, 2024

View reviewed changes

adarshsanjeev mentioned this pull request Jul 9, 2024

Add a segmentMorphFactory to MSQ Datasource Destination #16692

Closed

10 tasks

Improve coverage

ecdeee1

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

View reviewed changes

Comment thread processing/src/test/java/org/apache/druid/common/semantic/SemanticUtilsTest.java Fixed

adarshsanjeev added 2 commits July 10, 2024 10:47

Improve coverage

5dd3090

Fix test

a27894f

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

View reviewed changes

Comment thread processing/src/test/java/org/apache/druid/common/semantic/SemanticUtilsTest.java

}

@SemanticCreator

public AInterface toAInterface(String invalidParameter)

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'invalidParameter' is never used.

adarshsanjeev added 3 commits July 19, 2024 11:25

Refactor simpleQueryableIndex into an abstract class

8b72e9e

Fix checkstyle

44824b1

Add tests

3b834d2

kgyrtkirk reviewed Jul 22, 2024

View reviewed changes

cheddar merged commit 2b81c18 into apache:master Aug 6, 2024

clintropolis mentioned this pull request Sep 28, 2024

read metadata in SimpleQueryableIndex if available to compute segment ordering #17181

Merged

kfaraz added this to the 31.0.0 milestone Oct 4, 2024

Conversation

adarshsanjeev commented Jul 8, 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!

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!

kgyrtkirk Jul 8, 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!

imply-cheddar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

imply-cheddar commented Jul 9, 2024

Uh oh!

Uh oh!

Check notice

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!

cheddar commented Aug 6, 2024

Uh oh!

adarshsanjeev commented Jul 8, 2024 •

edited

Loading

kgyrtkirk Jul 8, 2024 •

edited

Loading