kgyrtkirk commented on code in PR #15371:
URL: https://github.com/apache/druid/pull/15371#discussion_r1410684788
##########
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:
I believe there are two ways to provide a `ColumnValueSelector` for `Object`
-t:
* one which doesn't want to be treated like something which could answer a
call like `getLong()` or `isNull()` as it wants to enforce that people will use
the `getObject()` call
* another one which provides the `getLong` and other methods based on a
backing `getObject` which *could* be even a primitive
The old `ObjectColumnSelector` was kinda tring to do both of the above; in
the latest changeset I've modified it to simply throw an Exception for every
`getX` call to prevent incorrect usage - I needed to make some changes to some
tests as they were asserting on the `isNull` method (which was implemented by
`return false`).
I've added ta second class which does the 2nd kind of thing named
`ObjectBasedColumnSelector` - and its used in the windowing stuff and a few
other places; I don't see much value of copy-pasting that implementation to
every place as if you have an array of objects. If someone might want to call
`getLong()` on it there is not really a alternate ways...if this ends up being
slow or something - and identified as a performance bottleneck...we could roll
`ArrayBasedTypeColumnSelector` classes via a strategy pattern and done...but
right now I doubt that around windowing this would be the 1st bottleneck :D
--
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]
