gianm commented on code in PR #18078:
URL: https://github.com/apache/druid/pull/18078#discussion_r2130579537
##########
processing/src/main/java/org/apache/druid/segment/AutoTypeColumnIndexer.java:
##########
@@ -444,10 +444,7 @@ public ColumnType getLogicalType()
}
if (fieldIndexers.size() == 1 &&
fieldIndexers.containsKey(NestedPathFinder.JSON_PATH_ROOT)) {
FieldIndexer rootField =
fieldIndexers.get(NestedPathFinder.JSON_PATH_ROOT);
- ColumnType logicalType = null;
- for (ColumnType type :
FieldTypeInfo.convertToSet(rootField.getTypes().getByteValue())) {
- logicalType = ColumnType.leastRestrictiveType(logicalType, type);
- }
+ ColumnType logicalType =
ColumnType.leastRestrictiveType(FieldTypeInfo.convertToSet(rootField.getTypes().getByteValue()));
Review Comment:
Are all the changes like this one related to the patch / fixing anything, or
just general code cleanup?
##########
processing/src/main/java/org/apache/druid/segment/nested/NestedFieldDictionaryEncodedColumn.java:
##########
@@ -163,11 +159,11 @@ public IndexedInts getMultiValueRow(int rowNum)
public String lookupName(int id)
{
final int globalId = dictionary.get(id);
- if (globalId < globalDictionary.size()) {
Review Comment:
are these related / fixing anything? Or just some code cleanup? Same
question for the similar changes in `lookupGlobalScalarObject`.
##########
processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java:
##########
@@ -534,4 +467,63 @@ public static Object coerceEvalToObjectOrList(ExprEval
eval)
}
return eval.valueOrDefault();
}
+
+ /**
+ * Wraps a {@link ColumnValueSelector<ExprEval>} and calls {@link
ExprEval#valueOrDefault()} on the output of
+ * {@link #baseSelector#getObject()} in {@link #getObject()}.
+ */
+ private static class EvalUnwrappingColumnValueSelector implements
ColumnValueSelector
+ {
+ private final ColumnValueSelector<ExprEval> baseSelector;
+
+ public EvalUnwrappingColumnValueSelector(ColumnValueSelector<ExprEval>
baseSelector)
+ {
+ this.baseSelector = baseSelector;
+ }
+
+ @Override
+ public double getDouble()
+ {
+ return baseSelector.getDouble();
+ }
+
+ @Override
+ public float getFloat()
+ {
+ return baseSelector.getFloat();
+ }
+
+ @Override
+ public long getLong()
+ {
+ return baseSelector.getLong();
+ }
+
+ @Override
+ public boolean isNull()
+ {
+ return baseSelector.isNull();
+ }
+
+ @Nullable
+ @Override
+ public Object getObject()
+ {
+ // No need for null check on getObject() since baseSelector impls will
never return null.
+ ExprEval eval = baseSelector.getObject();
+ return eval.valueOrDefault();
Review Comment:
`valueOrDefault` is pointless now, right? I think it should be the same as
`eval.value()` since the removal of legacy null handling.
##########
processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java:
##########
@@ -169,19 +93,28 @@ public Object getObject()
ExprEval eval = baseSelector.getObject();
return coerceEvalToObjectOrList(eval);
}
+ };
+ }
+ public static ColumnValueSelector castColumnValueSelector(
Review Comment:
Some javadoc would be useful here. Also I'm wondering which parameters are
nullable, if any? (Can the `delegateType` be null or must it be known?)
--
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]
