kgyrtkirk commented on code in PR #16700:
URL: https://github.com/apache/druid/pull/16700#discussion_r1686095581
##########
processing/src/main/java/org/apache/druid/segment/data/BlockLayoutColumnarLongsSupplier.java:
##########
@@ -169,7 +175,7 @@ public void get(final long[] out, final int start, final
int length)
}
final int limit = Math.min(length - p, sizePer - bufferIndex);
- reader.read(out, p, bufferIndex, limit);
Review Comment:
are there any tests which are passing not `0` as offset?
##########
processing/src/main/java/org/apache/druid/segment/data/ColumnarInts.java:
##########
@@ -28,4 +28,10 @@
*/
public interface ColumnarInts extends IndexedInts, Closeable
{
+ default void get(int[] out, int offset, int start, int length)
Review Comment:
are there any benefit of implementing this method here - and not in
`IndexedInts` which already has a `get` method?
##########
processing/src/test/java/org/apache/druid/segment/IndexMergerLongestSharedDimOrderTest.java:
##########
@@ -167,11 +167,15 @@ private QueryableIndexIndexableAdapter
makeIndexWithDimensionList(List<String> d
mockBitmapFactory,
ImmutableMap.of(ColumnHolder.TIME_COLUMN_NAME, mockSupplier),
mockSmooshedFileMapper,
- null,
true
)
+ {
+ @Override
+ public Metadata getMetadata()
+ {
+ return null;
Review Comment:
if its valid to have `null` metadata; it could be the `default`
implementation in the interface
##########
processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java:
##########
@@ -62,6 +67,12 @@ default void get(long[] out, int[] indexes, int length)
@Override
void close();
+ @Nullable
+ default <T> T as(Class<? extends T> clazz)
Review Comment:
since this PR is about to make the `SemanticCreator` more mainstream: could
you please introduce an interface for it and use it everywhere ?
##########
processing/src/main/java/org/apache/druid/common/semantic/SemanticUtils.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.common.semantic;
+
+import org.apache.druid.error.DruidException;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.function.Function;
+
+public class SemanticUtils
Review Comment:
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
##########
processing/src/main/java/org/apache/druid/segment/IndexIO.java:
##########
@@ -663,9 +650,32 @@ public QueryableIndex load(File inDir, ObjectMapper
mapper, boolean lazy, Segmen
segmentBitmapSerdeFactory.getBitmapFactory(),
columns,
smooshedFiles,
- metadata,
lazy
- );
+ )
+ {
+ @Override
+ public Metadata getMetadata()
+ {
+ try {
+ ByteBuffer metadataBB = smooshedFiles.mapFile("metadata.drd");
Review Comment:
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?
##########
processing/src/main/java/org/apache/druid/common/semantic/SemanticUtils.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.common.semantic;
+
+import org.apache.druid.error.DruidException;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.function.Function;
+
+public class SemanticUtils
+{
+ private static final Map<Class<?>, Map<Class<?>, Function<?, ?>>> OVERRIDES
= new LinkedHashMap<>();
Review Comment:
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?
##########
processing/src/main/java/org/apache/druid/segment/column/LongsColumn.java:
##########
@@ -75,6 +77,13 @@ public long getLongSingleValueRow(int rowNum)
return column.get(rowNum);
}
+ @Override
+ @Nullable
+ public <T> T as(Class<? extends T> clazz)
+ {
+ return column.as(clazz);
Review Comment:
this is a non-overridable as method....or not?
how will I be able to override this with `SemanticUtils.registerOverride`?
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/FrameMaker.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.semantic;
+
+import org.apache.druid.frame.Frame;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.segment.column.RowSignature;
+
+public interface FrameMaker
Review Comment:
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`
##########
processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java:
##########
@@ -46,9 +46,14 @@ public interface ColumnarLongs extends Closeable
long get(int index);
default void get(long[] out, int start, int length)
+ {
+ get(out, 0, start, length);
+ }
+
+ default void get(long[] out, int offset, int start, int length)
Review Comment:
what's driving this change?
why the need for this here and not in other classes like `ColumnarDoubles` ?
##########
processing/src/main/java/org/apache/druid/segment/column/BaseColumn.java:
##########
@@ -41,4 +42,11 @@ default VectorObjectSelector
makeVectorObjectSelector(ReadableVectorOffset offse
{
throw new UOE("Cannot make VectorObjectSelector for column with
class[%s]", getClass().getName());
}
+
+ @SuppressWarnings("unused")
+ @Nullable
+ default <T> T as(Class<? extends T> clazz)
+ {
+ return null;
Review Comment:
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`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
