imply-cheddar commented on code in PR #16700: URL: https://github.com/apache/druid/pull/16700#discussion_r1669710600
########## 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<>(); + + @SuppressWarnings("unused") + public static <C, T> void registerAsOverride(Class<C> clazz, Class<T> asInterface, Function<C, T> fn) Review Comment: It fundamentally exists for extensions to add things on their own, so it might appear to be unused for now. ########## processing/src/main/java/org/apache/druid/segment/nested/CompressedNestedDataComplexColumn.java: ########## @@ -534,9 +537,14 @@ public ColumnValueSelector<?> makeColumnValueSelector(List<NestedPathPart> path, if (arrayFieldIndex >= 0) { final int elementNumber = ((NestedPathArrayElement) lastPath).getIndex(); if (elementNumber < 0) { - throw new IAE("Cannot make array element selector for path [%s], negative array index not supported for this selector", path); + throw new IAE( + "Cannot make array element selector for path [%s], negative array index not supported for this selector", + path); Review Comment: Bit of a nit, but I just noticed that this is still throw IAE, it would probably be good to switch these to user-facing DruidExceptions as I think that all of these usages are expecting to get back to the user. ########## 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: This exists to allow extensions to register new interfaces and implementations without needing to impact the core code. This is probably worthy of javadoc. ########## processing/src/main/java/org/apache/druid/segment/SimpleQueryableIndex.java: ########## @@ -46,8 +47,8 @@ public class SimpleQueryableIndex implements QueryableIndex private final BitmapFactory bitmapFactory; private final Map<String, Supplier<ColumnHolder>> columns; private final SmooshedFileMapper fileMapper; - @Nullable - private final Metadata metadata; + @NotNull + private final Supplier<Metadata> metadataSupplier; Review Comment: 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. ########## 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: 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. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
