Support UDF in planner #2995
Support UDF in planner #2995
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
|
fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/Key.java
Show resolved
Hide resolved
...-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/MacroFunctionTest.java
Show resolved
Hide resolved
| @@ -20,9 +20,9 @@ | |||
|
|
|||
| package com.apple.foundationdb.relational.util; | |||
|
|
|||
| import com.apple.foundationdb.annotation.API; | |||
| import static com.apple.foundationdb.record.expressions.RecordKeyExpressionProto.*; | |||
There was a problem hiding this comment.
use static import to make file more readable.
There was a problem hiding this comment.
checkstyle doesn't allow "*" in imports, reverted the change.
There was a problem hiding this comment.
You can static import them separately. Sometimes, IntelliJ is configured to group them into a * import.
There was a problem hiding this comment.
right, but unfortunately build still fails because checkStyle doesn't allow * in import lines.
| return MacroFunction.fromProto(serializationContext, proto.getMacroFunction()); | ||
| } else { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
can add more if blocks when we have more items in the oneof
There was a problem hiding this comment.
will do dynamic dispatch in a follow-up PR
| } | ||
|
|
||
| @Nonnull | ||
| public static MacroFunction fromProto(@Nonnull final PlanSerializationContext serializationContext, @Nonnull final PMacroFunctionValue functionValue) { |
There was a problem hiding this comment.
I understand that I can inject serializationContext with @autoservice, but this method is called from RecordMetaDataBuilder, which doesn't have serializationContext injected. So it seems that I'll need to create a serializationContext in RecordMetaDataBuilder and pass it to MacroFunction.fromProto anyways.
There was a problem hiding this comment.
The context is normally used to do dynamic dispatch among the implementing classes.
There was a problem hiding this comment.
Got it. will do it in a follow-up PR.
| */ | ||
| public class MacroFunction implements SerializableFunction { | ||
| @Nonnull | ||
| private final String functionName; |
There was a problem hiding this comment.
All functions will have this in common:
- function name
- declared parameters (types at least)
- return type
Can you move that up into SerializableFunction? You can give SerializableFunction the appropriate getters forcing all implementors to provide methods for them. As a second step, have an abstract base class implementing SerializableFunction. Let's call that AbstractCatalogedFunction. All your methods extends from that. It's the same approach Value, and AbstractValue (for instance) have taken.
There was a problem hiding this comment.
done, restructured the code.
| import javax.annotation.Nonnull; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| public interface SerializableFunction { |
There was a problem hiding this comment.
Can this be called something that encompasses more than just serialization. I had suggested CatalogedFunction.
There was a problem hiding this comment.
discussed offline, renamed.
| * limitations under the License. | ||
| */ | ||
|
|
||
| package com.apple.foundationdb.record.metadata; |
There was a problem hiding this comment.
This approach you have taken is not what I had in mind:
BuiltInFunctionshould give up most of its attributes/functionality, encapsulation to this class. Only the encapsulation lambda should stay that is used to implement theencapsulate()method.BuiltInFunctionshould be THE base for all internal functions that are not domain specific (likelength(string)or similar).MacroFunctionshould be extending this class and adding the functionality that you coded up to expand an arbitrary body
There was a problem hiding this comment.
discussed offline, restructured the code.
| } | ||
|
|
||
| @Nonnull | ||
| public static MacroFunction fromProto(@Nonnull final PlanSerializationContext serializationContext, @Nonnull final PMacroFunctionValue functionValue) { |
There was a problem hiding this comment.
The context is normally used to do dynamic dispatch among the implementing classes.
| } | ||
|
|
||
| @Nullable | ||
| public static UserDefinedFunction fromProto(@Nonnull PlanSerializationContext serializationContext, @Nonnull RecordMetaDataProto.UserDefinedFunction proto) { |
There was a problem hiding this comment.
You can autoservice the extends for UserDefinedFunction and then call the appropriate dispatch routine using the serialization context. We currently do this for Value, RecordQueryPlan, and for Type.
There was a problem hiding this comment.
will do the dispatch in a follow-up PR.
No description provided.