Support UDF in plan generator #3040
Conversation
This reverts commit 3a5a44b.
Result of fdb-record-layer-pr on Linux CentOS 7
|
Result of fdb-record-layer-pr on Linux CentOS 7
|
There was a problem hiding this comment.
I am putting @alecgrieser on this review as well to make sure the proto layout and dependencies are what we want. I left a bunch of comments. There is some more work required but it's on the right track. I would recommend not ever removing -Werror (not even in your local build). It is useful. Also, please make sure the PR runs through the PRB.
| <outputRelativeToContentRoot value="true" /> | ||
| <processorPath useClasspath="false"> | ||
| <entry name="$PROJECT_DIR$/fdb-java-annotations/.out/libs/fdb-java-annotations-3.5-SNAPSHOT.jar" /> | ||
| <entry name="$PROJECT_DIR$/fdb-java-annotations/.out/libs/fdb-java-annotations-4.0-SNAPSHOT.jar" /> |
There was a problem hiding this comment.
Please remove this file from the change set.
| @@ -693,6 +705,7 @@ public RecordMetaDataProto.MetaData toProto(@Nullable Descriptors.FileDescriptor | |||
| } | |||
|
|
|||
| // Add in the final options. | |||
| builder.addAllScalarValuedFunction(scalarValuedFunctions.stream().map(ScalarValuedFunction::toProto).collect(Collectors.toList())); | |||
There was a problem hiding this comment.
This is not really a final option as the comment suggests.
There was a problem hiding this comment.
removed the confusing comment.
| @@ -0,0 +1,67 @@ | |||
| /* | |||
| * UDF.java | |||
There was a problem hiding this comment.
| * UDF.java | |
| * ScalarValuedFunction.java |
| import javax.annotation.Nonnull; | ||
|
|
||
| /** | ||
| * Defines a scalar User-defined-function. |
There was a problem hiding this comment.
Please expand on the documentation.
| @@ -0,0 +1,121 @@ | |||
| /* | |||
| * BuiltInFunction.java | |||
There was a problem hiding this comment.
| * BuiltInFunction.java | |
| * Function.java |
|
|
||
|
|
||
| @Nonnull | ||
| public PValue toValueProto(@Nonnull final PlanSerializationContext serializationContext) { |
There was a problem hiding this comment.
I think this should be removed (as well as the change in the record_query_plan.proto). MacroFunction is not a Value and therefore cannot be used in place of a PValue
| @@ -238,14 +238,14 @@ public GraphExpansion visitExpression(@Nonnull final NestingKeyExpression nestin | |||
| .add(parent.getFieldName()) | |||
| .build(); | |||
| if (NullableArrayTypeUtils.isArrayWrapper(nestingKeyExpression)) { | |||
| final RecordMetaDataProto.KeyExpression childProto = nestingKeyExpression.getChild().toKeyExpression(); | |||
| final RecordKeyExpressionProto.KeyExpression childProto = nestingKeyExpression.getChild().toKeyExpression(); | |||
There was a problem hiding this comment.
If ambiguity permits, maybe just import those statically.
There was a problem hiding this comment.
unfortunately this file also has metadata.expressions.KeyExpression, so we can't remove the RecordKeyExpressionProto here.
| /** | ||
| * Defines a scalar User-defined-function. | ||
| */ | ||
| public class ScalarValuedFunction { |
There was a problem hiding this comment.
Why don't we just make a trait-like interface called SerializableFunction that makes this proto-serializable? MacroFunction can then implement SerializableFunction Then we don't have to wrap the MacroFunction here. It would also allow us to
support other functions in the future that are not MacroFunction. It's pretty straightforward for toProto(...), slightly less so on the way back. Please look how the plan serialization achieves the same thing by using:
oneofin the proto declarationsPlanDeserializerusingAutoServiceto get the dispatch of the from proto part right
| NULL(RecordMetaDataProto.Field.NullInterpretation.NOT_UNIQUE), // Missing field here skips uniqueness checks. | ||
| NULL_UNIQUE(RecordMetaDataProto.Field.NullInterpretation.UNIQUE), // Missing field here like ordinary value, but null, for uniqueness. | ||
| NOT_NULL(RecordMetaDataProto.Field.NullInterpretation.NOT_NULL); // Missing field has type's ordinary default value. | ||
| NULL(RecordKeyExpressionProto.Field.NullInterpretation.NOT_UNIQUE), // Missing field here skips uniqueness checks. |
There was a problem hiding this comment.
static imports may make this more readable.
There was a problem hiding this comment.
unfortunately this file also has metadata.expressions.KeyExpression.
| @@ -131,6 +136,7 @@ protected RecordMetaData(@Nonnull Descriptors.FileDescriptor recordsDescriptor, | |||
| @Nonnull Map<String, Index> indexes, | |||
| @Nonnull Map<String, Index> universalIndexes, | |||
| @Nonnull List<FormerIndex> formerIndexes, | |||
| @Nonnull Set<ScalarValuedFunction> scalarValuedFunctions, | |||
There was a problem hiding this comment.
There are a few different comments further down in this PR around this. Let me just point out here that this structure is persisted and long-lived. In fact, modifying its contents and layout may be impossible or very hard once this is used. One thing I would like to avoid is to get ourselves into a dead end here.
We most likely want to support other functions-like objects than macros. For instance, we would like to use full SQL-bodied functions or table functions. This attribute here should potentially be able to contain all of these. Using my naming from a files down, this should be something named SerializableFunction (because that's all you need here, i.e. a name, a bunch of parameter declarations with their types and a return type). Under the hood it can be any of the particular flavors I mentioned (of which we only have a simple MacroFunction as of yet).
Result of fdb-record-layer-pr on Linux CentOS 7
|
No description provided.