clintropolis commented on code in PR #15371: URL: https://github.com/apache/druid/pull/15371#discussion_r1416356342
########## processing/src/main/java/org/apache/druid/segment/ObjectBasedColumnSelector.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.segment; + +import org.apache.druid.error.DruidException; + +/** + * Object based column selector. + * + * This abstract class provides a {@link ColumnValueSelector} based on the methods of {@link BaseObjectColumnValueSelector}. + */ +public abstract class ObjectBasedColumnSelector<T> implements ColumnValueSelector<T> +{ + private static final String COLUMN_IS_NULL_ERROR_MESSAGE = "Invalid usage pattern: method returning primitive called - but the column is null"; + + @Override + public final float getFloat() + { + T value = getObject(); + if (value == null) { + throw DruidException.defensive(COLUMN_IS_NULL_ERROR_MESSAGE); + } + return ((Number) value).floatValue(); + } + + @Override + public final double getDouble() + { + T value = getObject(); + if (value == null) { + throw DruidException.defensive(COLUMN_IS_NULL_ERROR_MESSAGE); + } + return ((Number) value).doubleValue(); + } + + @Override + public final long getLong() + { + T value = getObject(); + if (value == null) { + throw DruidException.defensive(COLUMN_IS_NULL_ERROR_MESSAGE); + } + return ((Number) value).longValue(); + } + + @Override + public final boolean isNull() + { + return getObject() == null; Review Comment: I think this should also probably be checking that the result of `getObject()` is `instanceof Number`. The things that are going to call `isNull()` will do so in service of calling it prior to calling `getLong`/`getFloat`/`getDouble`, but if Object is not a `Number` then this will result in a class cast exception and there is no restriction on this class that the things it provides are always instances of a `Number`. Things that are calling `getObject` should already know not to be checking `isNull` for null checks, so I don't think that would cause any problems if `isNull` returns true but `getObject` returns a non-null value. ########## processing/src/main/java/org/apache/druid/segment/ObjectColumnSelector.java: ########## @@ -19,75 +19,36 @@ package org.apache.druid.segment; +import org.apache.druid.error.DruidException; + /** - * This class is convenient for implementation of "object-sourcing" {@link ColumnValueSelector}s, it provides default - * implementations for all {@link ColumnValueSelector}'s methods except {@link #getObject()} and {@link - * #classOfObject()}. - * - * This class should appear ONLY in "extends" clause or anonymous class creation, but NOT in "user" code, where - * {@link BaseObjectColumnValueSelector} must be used instead. - * - * This is a class rather than an interface unlike {@link LongColumnSelector}, {@link FloatColumnSelector} and {@link - * DoubleColumnSelector} solely in order to make {@link #isNull()} method final. + * Restricts selector usage to only allow {@link #getObject()}. */ public abstract class ObjectColumnSelector<T> implements ColumnValueSelector<T> Review Comment: I think might have gone a bit overboard in removing the javadoc contract of this class, some of that stuff still seems relevant about only extending it ########## processing/src/main/java/org/apache/druid/segment/ObjectBasedColumnSelector.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.segment; + +import org.apache.druid.error.DruidException; + +/** + * Object based column selector. + * + * This abstract class provides a {@link ColumnValueSelector} based on the methods of {@link BaseObjectColumnValueSelector}. + */ +public abstract class ObjectBasedColumnSelector<T> implements ColumnValueSelector<T> Review Comment: I think we need a better name for this because this isn't strictly an object selector like `ObjectColumnSelector` is, and instead since it also implements `BaseLongColumnValueSelector`, `BaseDoubleColumnValueSelector`, `BaseFloatColumnValueSelector` and `BaseNullableColumnValueSelector`, so its sort of a multi-type selector. It is also important to call the contract of this class - that it doesn't do any real type coercion and only satisfies the numeric selectors if the value is already a `Number`. ########## processing/src/test/java/org/apache/druid/segment/NestedDataColumnIndexerV4Test.java: ########## @@ -551,7 +551,7 @@ public void testNestedColumnIndexerSchemaDiscoveryNested() throws IndexSizeExcee () -> cursorList.get(1).getColumnSelectorFactory().makeDimensionSelector(dimensionSpec) ); Assert.assertEquals(StructuredData.wrap(2L), valueSelector.getObject()); - Assert.assertFalse(valueSelector.isNull()); + Assert.assertNotNull(valueSelector.getObject()); Review Comment: same comment about removing check ########## processing/src/test/java/org/apache/druid/segment/AutoTypeColumnIndexerTest.java: ########## @@ -569,7 +569,7 @@ public void testNestedColumnIndexerSchemaDiscoveryNested() throws IndexSizeExcee () -> cursorList.get(2).getColumnSelectorFactory().makeDimensionSelector(dimensionSpec) ); Assert.assertEquals(StructuredData.wrap(ImmutableMap.of("x", 1.1, "y", 2L)), valueSelector.getObject()); - Assert.assertFalse(valueSelector.isNull()); + Assert.assertNotNull(valueSelector.getObject()); Review Comment: same comment about removing check ########## processing/src/test/java/org/apache/druid/segment/NestedDataColumnIndexerV4Test.java: ########## @@ -560,7 +560,7 @@ public void testNestedColumnIndexerSchemaDiscoveryNested() throws IndexSizeExcee () -> cursorList.get(2).getColumnSelectorFactory().makeDimensionSelector(dimensionSpec) ); Assert.assertEquals(StructuredData.wrap(ImmutableMap.of("x", 1.1, "y", 2L)), valueSelector.getObject()); - Assert.assertFalse(valueSelector.isNull()); + Assert.assertNotNull(valueSelector.getObject()); Review Comment: same comment about removing check ########## processing/src/test/java/org/apache/druid/segment/AutoTypeColumnIndexerTest.java: ########## @@ -560,7 +560,7 @@ public void testNestedColumnIndexerSchemaDiscoveryNested() throws IndexSizeExcee () -> cursorList.get(1).getColumnSelectorFactory().makeDimensionSelector(dimensionSpec) ); Assert.assertEquals(StructuredData.wrap(2L), valueSelector.getObject()); - Assert.assertFalse(valueSelector.isNull()); + Assert.assertNotNull(valueSelector.getObject()); Review Comment: i think this can probably just be removed, the object of this indexer will never be null because the value is always wrapped in `StructuredData` -- 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]
