clintropolis commented on code in PR #15371:
URL: https://github.com/apache/druid/pull/15371#discussion_r1409975644
##########
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 think this method should be either implemented correctly or throw an
exception that its not supported - that will make sure that there won't be
correctness issues if somehow it gets called
That is what i was getting at, if `getLong`/`getFloat`/`getDouble` are going
to throw exceptions, then this should too, since it should only be called if
you are planning on calling one of those methods.
>it is called from places like this - how should we fix that?
`NullableNumericAggregator` is _only_ for things that are going to be
calling `getLong`/`getFloat`/`getDouble`, which are the only things that are
supposed to call `isNull`, because those methods return primitives which cannot
be null. That aggregator should never be using a selector derived from this
one, since it is designed to aggregate primitive numeric values.
>I don't understand clearly what you mean by "changing the contract of
isNull" ; I believe that's defined
[here](https://github.com/apache/druid/blob/93cd63864581b16d6b9407723541616a7465da68/processing/src/main/java/org/apache/druid/segment/BaseNullableColumnValueSelector.java#L31-L42);
and returning false have kinda violated it
This does not violate it, because no one should _ever_ call
`getLong`/`getFloat`/`getDouble` on that value selector, which means they
should also not be using `isNull`, and should instead just call `getObject` and
see if the result is null. Calling `isNull` if you intend to call `getObject`
is violating the contract.
--
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]
