-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nested columns + arrays = array columns! #13803
Changes from all commits
a8bf82c
f66f0e5
1acdad4
eb6976a
524975b
dd4b899
d8e1b3f
8ff7933
4f72c47
0070157
ae382d0
6712e7a
26ebeef
2ecd8bd
7bfb113
9ea04a3
82f01aa
810649c
76da0fb
313f486
665e0bc
e3f48a7
e988d05
1515267
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -370,11 +370,26 @@ public static ExprEval ofComplex(ExpressionType outputType, @Nullable Object val | |
return new ComplexExprEval(outputType, value); | ||
} | ||
|
||
public static ExprEval bestEffortArray(@Nullable List<?> theList) | ||
{ | ||
// do not convert empty lists to arrays with a single null element here, because that should have been done | ||
// by the selectors preparing their ObjectBindings if necessary. If we get to this point it was legitimately | ||
// empty | ||
NonnullPair<ExpressionType, Object[]> coerced = coerceListToArray(theList, false); | ||
if (coerced == null) { | ||
return bestEffortOf(null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would probably be best to have null handled first in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I expected you to actually to create a method for |
||
} | ||
return ofArray(coerced.lhs, coerced.rhs); | ||
} | ||
|
||
/** | ||
* Examine java type to find most appropriate expression type | ||
*/ | ||
public static ExprEval bestEffortOf(@Nullable Object val) | ||
{ | ||
if (val == null) { | ||
return new StringExprEval(null); | ||
} | ||
if (val instanceof ExprEval) { | ||
return (ExprEval) val; | ||
} | ||
|
@@ -468,14 +483,7 @@ public static ExprEval bestEffortOf(@Nullable Object val) | |
|
||
if (val instanceof List || val instanceof Object[]) { | ||
final List<?> theList = val instanceof List ? ((List<?>) val) : Arrays.asList((Object[]) val); | ||
// do not convert empty lists to arrays with a single null element here, because that should have been done | ||
// by the selectors preparing their ObjectBindings if necessary. If we get to this point it was legitimately | ||
// empty | ||
NonnullPair<ExpressionType, Object[]> coerced = coerceListToArray(theList, false); | ||
if (coerced == null) { | ||
return bestEffortOf(null); | ||
} | ||
return ofArray(coerced.lhs, coerced.rhs); | ||
return bestEffortArray(theList); | ||
} | ||
|
||
// in 'best effort' mode, we couldn't possibly use byte[] as a complex or anything else useful without type | ||
|
@@ -485,12 +493,8 @@ public static ExprEval bestEffortOf(@Nullable Object val) | |
return new StringExprEval(StringUtils.encodeBase64String((byte[]) val)); | ||
} | ||
|
||
if (val != null) { | ||
// is this cool? | ||
return new ComplexExprEval(ExpressionType.UNKNOWN_COMPLEX, val); | ||
} | ||
|
||
return new StringExprEval(null); | ||
// is this cool? | ||
return new ComplexExprEval(ExpressionType.UNKNOWN_COMPLEX, val); | ||
} | ||
|
||
public static ExprEval ofType(@Nullable ExpressionType type, @Nullable Object value) | ||
|
@@ -1109,7 +1113,6 @@ private ArrayExprEval(ExpressionType arrayType, @Nullable Object[] value) | |
super(value); | ||
this.arrayType = arrayType; | ||
Preconditions.checkArgument(arrayType.isArray(), "Output type %s is not an array", arrayType); | ||
ExpressionType.checkNestedArrayAllowed(arrayType); | ||
} | ||
|
||
@Override | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change in expectation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is related to the change in dimension filter in
MapBasedInputRow
to always filter out the timestamp spec column.In 'normal' production code the timestamp spec is added to dimension exclusions so the code in
MapBasedInputRow
that computes dimensions to ensure time would not be in the dimensions list. However, in test code, especially in processing, which doesn't have access to the methods that take a dataschema and transform it into an input row schema, its pretty easy to not explicitly add timestamp column to the dimensions exclusion list. So as a result of not manually adding timestamp column to exclusions, it would end up in the dimensions list in schema discovery modes, as a string (or nested column, depending on config), which when doing rollup tests means it ends up as part of the rollup key, and so on. (Again this doesn't happen in current production code because it goes through that translator utility method).I made the change there to always filter the timestamp spec column from the dimensions list to make it easier to not write wrong tests for schema discovery mode, which caused the change here and other places.