kgyrtkirk commented on code in PR #15371:
URL: https://github.com/apache/druid/pull/15371#discussion_r1409993926
##########
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:
> 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.
I think it should throw an exception because `null` can't be represented as
a primitive; it could highlight that you should call `isNull` first - I can
revert that change; but I think those default values may only help to hide
bugs....
> [...] NullableNumericAggregator is only for things that are going to be
calling [...]
that sounds rather odd - there are no interfaces enforcing that contract; it
only requires that the `selector` implement `BaseNullableColumnValueSelector` ;
and because of that object which implement that should work with it
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)
>This does not violate it, because no one should ever call
"no one should ever call" : that can't be enforced; it
providing a `return false` implementation is wrong - and a violation of the
contract in this case.
> Calling isNull if you intend to call getObject is violating the contract.
now you lost me...then why implement `ColumnValueSelector` at all? or not
just throw exceptions from all of them?
this seems like some kind of design issue...
I think a possible alternate way to fix this could be to *not* implement
`isNull` in `ObjectColumnSelector` and force classes extending it to provide
their own implementation - what do you think ?
--
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]
