Skip to content

[WIP] Interface for RowExpression translation#13483

Closed
sachdevs wants to merge 1 commit intoprestodb:masterfrom
sachdevs:connector-optimizer-translation-interface
Closed

[WIP] Interface for RowExpression translation#13483
sachdevs wants to merge 1 commit intoprestodb:masterfrom
sachdevs:connector-optimizer-translation-interface

Conversation

@sachdevs
Copy link
Contributor

A bare bones mocked implementation of how the interface for row expression translation in the connector may look like.

Details are still being finalized, we may/may not agree to do it this way. Read my review comments for any details on what this is supposed to be doing.

== NO RELEASE NOTE ==


import java.util.Map;

public abstract class AbstractExpressionTranslator<T>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User will extend this class when writing their translator. It will be populated with default implementations.

}

@Override
public TargetExpression<T> translateConstantExpression(ConstantExpression constantExpression, Map context)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mocked functions, will implement once we get consensus on how this interface should look.


import java.util.Map;

public class RowExpressionTranslator<T>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User uses this class in their ComputePushdown class (see JdbcComputePushdown.java) and calls .translate on it.


import static java.util.Objects.requireNonNull;

class RowExpressionTranslatorVisitor<T>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Underlying adapter for RowExpression visitor. This calls functions on the ExpressionTranslator interface. This class allows to hide internals from the user (i.e. they don't have to care about the visitor, only have to implement functions that are pertinent to their use case)


import static com.facebook.presto.translator.registry.ScalarFromAnnotationsParser.removeTypeParameters;

public class ConstantTypeRegistry
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For registering constant types. May consider combining Constant and Function registries. Lmk if a better way to do this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may not need this.

{
private final ImmutableMap<FunctionMetadata, MethodHandle> translators;

public FunctionRegistry(Class<?>... classes)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For registering MethodHandles to their corresponding function metadata so that they can be invoked via reflection.

Copy link
Contributor Author

@sachdevs sachdevs Sep 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AbstractExpressionTranslator will use this class and ConstantTypeRegistry for it's default implementations of translate functions.

import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

public class ScalarFromAnnotationsParser
Copy link
Contributor Author

@sachdevs sachdevs Sep 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from original annotation parsing classes in Scuba/Cubrick

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to check how much we can reuse the existing ScalarFromAnnotationsParser. Most of the code paths are duplicated

import static com.google.common.base.Preconditions.checkArgument;
import static java.util.Objects.requireNonNull;

public class ScalarHeader
Copy link
Contributor Author

@sachdevs sachdevs Sep 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from original annotation parsing classes in Scuba/Cubrick

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this. Use with existing ScalarHeader


public interface ExpressionTranslator<T>
{
TargetExpression<T> translateCallExpression(CallExpression callExpression, Map<VariableReferenceExpression, ColumnHandle> context);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Primary way user will interact with the translation interface.

private List<TargetExpression> params;
private Optional<T> translatedExpression;

public TargetExpression(Optional<T> translatedExpression)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rough implementation of TargetExpression, the fields are accurate but most of these methods will be changed.

public abstract class AbstractExpressionTranslator<T>
implements ExpressionTranslator
{
private final FunctionRegistry functionRegistry;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be protected/ have protected accessors.

Copy link

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c36a9aa is an example code framework that could help

import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

public class ScalarFromAnnotationsParser
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to check how much we can reuse the existing ScalarFromAnnotationsParser. Most of the code paths are duplicated

import static com.google.common.base.Preconditions.checkArgument;
import static java.util.Objects.requireNonNull;

public class ScalarHeader
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this. Use with existing ScalarHeader


import java.util.Map;

public interface ExpressionTranslator<T>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should build something like RowExpressionRewriter and RowExpressionTreeRewriter to as a helper


import static com.facebook.presto.translator.registry.ScalarFromAnnotationsParser.removeTypeParameters;

public class ConstantTypeRegistry
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may not need this.


import static com.facebook.presto.translator.registry.ScalarFromAnnotationsParser.parseFunctionDefinitions;

public class FunctionRegistry
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check my example code

import java.util.List;
import java.util.Optional;

public class TargetExpression<T>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put this together with optimizer interface.

@sachdevs
Copy link
Contributor Author

sachdevs commented Oct 2, 2019

#13491 continue work here.

@sachdevs sachdevs closed this Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants