clintropolis commented on code in PR #15371:
URL: https://github.com/apache/druid/pull/15371#discussion_r1410021613
##########
processing/src/main/java/org/apache/druid/segment/ObjectColumnSelector.java:
##########
@@ -19,75 +19,50 @@
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.
+ * Object based column selector.
*
- * 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.
+ * This abstract class provides a {@link ColumnValueSelector} based on the
methods of {@link BaseObjectColumnValueSelector}.
*/
public abstract class ObjectColumnSelector<T> implements ColumnValueSelector<T>
{
- /**
- * @deprecated This method is marked as deprecated in ObjectColumnSelector
to minimize the probability of accidental
- * calling. "Polymorphism" of ObjectColumnSelector should be used only when
operating on {@link ColumnValueSelector}
- * objects.
- */
- @Deprecated
+ private static final String COLUMN_IS_NULL_ERROR_MESSAGE = "Invalid usage
pattern: method returning primitive called - but the column is null";
+
@Override
- public float getFloat()
+ public final float getFloat()
{
T value = getObject();
if (value == null) {
- return 0;
+ throw DruidException.defensive(COLUMN_IS_NULL_ERROR_MESSAGE);
}
return ((Number) value).floatValue();
}
- /**
- * @deprecated This method is marked as deprecated in ObjectColumnSelector
to minimize the probability of accidental
- * calling. "Polymorphism" of ObjectColumnSelector should be used only when
operating on {@link ColumnValueSelector}
- * objects.
- */
- @Deprecated
@Override
- public double getDouble()
+ public final double getDouble()
{
T value = getObject();
if (value == null) {
- return 0;
+ throw DruidException.defensive(COLUMN_IS_NULL_ERROR_MESSAGE);
}
return ((Number) value).doubleValue();
}
- /**
- * @deprecated This method is marked as deprecated in ObjectColumnSelector
to minimize the probability of accidental
- * calling. "Polymorphism" of ObjectColumnSelector should be used only when
operating on {@link ColumnValueSelector}
- * objects.
- */
- @Deprecated
@Override
- public long getLong()
+ public final long getLong()
{
T value = getObject();
if (value == null) {
- return 0;
+ throw DruidException.defensive(COLUMN_IS_NULL_ERROR_MESSAGE);
}
return ((Number) value).longValue();
}
- /**
- * @deprecated This method is marked as deprecated in ObjectColumnSelector
since it always returns false.
- * There is no need to call this method.
- */
- @Deprecated
@Override
public final boolean isNull()
{
- return false;
+ return getObject() == null;
Review Comment:
The current design is not great because it cannot enforce how callers use
stuff, I agree completely and have some ambition of reworking how this stuff
works to drop all of this polymorphism if possible, but its also a gigantic
task since `ColumnValueSelector` is use in a ton of places, so it will take
some time.
`ObjectColumnSelector` picks up `BaseNullableColumnValueSelector` because
`BaseLongColumnValueSelector` etc implement it. It seems controversial to me
that they provide default implementations of the get primitive methods at all
because it seems like it is easy to misuse.
The javadocs of `BaseNullableColumnValueSelector` explain the current
contract of `isNull`, and specify that things calling `getObject` should not
call `isNull` because it is not for them.
I actually kind of think maybe `ObjectColumnSelector` should throw
exceptions by default for `getLong`/getDouble`/`getFloat`/`isNull`, because it
isn't really intended to be used by things which should also maybe be long or
float selectors, but i have no idea how many things that might break. For
`ColumnValueSelector` that _do_ need to act as all types of selectors
potentially, such as the example you linked:
>This ObjectColumnSelector class provides getLong and friends based on
getObject which is exactly how its services are used
[here](https://github.com/apache/druid/blob/0516d0dae432058848806e49e71ac73333bf751c/processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DefaultFramedOnHeapAggregatable.java#L554-L575)
They should probably implement all of the methods themselves, because that
is not that common of a pattern and only happens in some special cases. Usually
`ColumnValueSelector` are specialized for the type they are selecting over.
--
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]
