ConnectorExpression pushdown

[assaf2]

For #18

[cla-bot]

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Assaf Bern.
This is most likely caused by a git client misconfiguration; please make sure to:

1. check if your git client is configured with an email to sign commits git config --list | grep email
2. If not, set it up using git config --global user.email email@example.com
3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

[alec-heif]

hi @assaf2 are you still planning to work on this PR? this would be very useful for my organization for being able to push down more complex queries into postgres.

[assaf2]

hi @assaf2 are you still planning to work on this PR? this would be very useful for my organization for being able to push down more complex queries into postgres.

@alec-heif good to hear, yes I'm on it

[findepi]

@martint do you have a doc or some writeup outlining your thinking about desired end-goal for ConnectorExpression-based pushdowns, especially predicate pushdown?

Besides the end-goal I would love to help us define intermediate steps that will allow us split the work into smaller pieces, without boiling the ocean.
It seems to me the ideal scenario is when we can gradually migrate from TupleDomain to ConnectorExpression
This requires to marry the two (e.g. be able to embed TupleDomain or Domain in ConnectorExpression.

Defining those intermediate steps should not only help reach the end-goal sooner, but also would allow deliver partial value to users as we go.

cc @losipiuk

[rzeyde-varada]

Greetings,
We'll be happy to help with the development of this feature - so please let us know the next implementation steps.
Our use case is complex predicates' pushdown (e.g. using string-related functions over some of the columns):
https://trinodb.slack.com/archives/CGJS76ZSR/p1607419450007700

[willzgw]

Hi everyone, may I know where we are with this PR?
Recently, we have a requirement to push time_floor(__time, 'period') down to Druid queries.
I did some research and found that we need a new class like PushAggregationIntoTableScan to support this feature.
Maybe I'm in a wrong way. I am not sure.
Could I get your guidance?

[martint]

Obtain the type object from TypeManager via getType(Re2JRegexpType.RE2J_REGEXP_SIGNATURE) instead of constructing an instance manually.

[assaf2]

I've tried to add TypeManager to PlanOptimizers's @Inject constructor and to pass it from there. I had to change ~15 files including tests, but then I got a runtime error since PlanOptimizers is bound at CoordinatorModule, while TypeManager is not bound yet.
I couldn't find another way to obtain TypeManager, any suggestions?

[martint]

This is what we need to implement this more generically, without having to hardcode any knowledge about how regexp functions work:

protected Optional<Expression> translateCall(Call call)
{
    if (LIKE_FUNCTION_NAME.equals(call.getName())) {
        return translateLike(call.getArguments().get(0), call.getArguments().get(1));
    }

    QualifiedName name = QualifiedName.of(call.getName());
    FunctionCallBuilder builder = new FunctionCallBuilder(metadata)
            .setName(name);

    List<TypeSignature> argumentTypes = call.getArguments().stream()
            .map(argument -> argument.getType().getTypeSignature())
            .collect(toCollection(ArrayList::new));

    ResolvedFunction resolved = metadata.resolveFunction(name, TypeSignatureProvider.fromTypeSignatures(argumentTypes));
    for (int i = 0; i < call.getArguments().size(); i++) {
        Expression expression = ConnectorExpressionTranslator.translate(session, call.getArguments().get(i), metadata, variableMappings, literalEncoder);
        Type expectedType = resolved.getSignature().getArgumentTypes().get(i);
        Type actualType = metadata.getType(argumentTypes.get(i));

        if (!actualType.equals(expectedType)) {
            verify(typeCoercion.canCoerce(actualType, expectedType));
            expression = new Cast(expression, toSqlType(expectedType));
        }

        builder.addArgument(expectedType, expression);
    }

    return Optional.of(builder.build());
}

[assaf2]

Thanks!
I just made a single change to your suggestion - replaced if (!actualType.equals(expectedType)) { with if (!typeCoercion.isTypeOnlyCoercion(actualType, expectedType)) { because, for example, at
testCrossJoinLargeBuildSideDynamicFiltering() we get $internal$dynamic_filter_function with a varchar(9) argument while its expectedType = varchar. Is this change ok by you?

In addition, are you aware of an equivalently generic way to implement io.trino.sql.planner.ConnectorExpressionTranslator.SqlToConnectorExpressionTranslator#visitFunctionCall which still refers to JoniRegexp and Re2JRegexp specifically?

[martint]

This translation can be done inside the if block below.

[assaf2]

The translator currently supports many functions.
If I'll do so, I'll need to create a FunctionCall only inside the if, so only regexp_like and LIKE functions will be supported. Therefore, I'll also need to change io.trino.sql.planner.ConnectorExpressionTranslator.SqlToConnectorExpressionTranslator#visitFunctionCall to convert only regexp functions (because otherwise we'll translate only in one direction).
Do you want me to narrow the translation this way?

[jerryleooo]

This for the hard work, not sure if #9067 belongs to this issue?

[slhmy]

I noticed that in prestodb/presto#16412. PrestoDB already realized part of expression pushdown for jdbc, some of the code is worth referenced.

[findepi]

@slhmy there is a follow-up with initial JDBC coverage: #11045

[slhmy]

@slhmy there is a follow-up with initial JDBC coverage: #11045

@findepi LGTM, rewriter's design seems to be better than my personal implementation!

[martint]

If connectorExpression is empty (i.e., either there wasn't a expression in the first place or the translation from Expression->ConnectorExpression failed), it is still possible that remainingConnectorExpression is not empty (i.e., if the connector synthesizes an expression). In that case, we need to combine decomposedPredicate.getRemainingExpression() with the result from applyFilter().

[assaf2]

Modified

[findepi]

( followed up here: #7994 (comment) )

[findepi]

The conversion

 Optional<ConnectorExpression> connectorExpression = new ConnectorExpressionTranslator.SqlToConnectorExpressionTranslator(session, remainingExpressionTypes, plannerContext)
                .process(decomposedPredicate.getRemainingExpression());

is lossy.

i.e. decomposedPredicate.getRemainingExpression() contains full information
but then connectorExpression contains partial information
remainingConnectorExpression contains part of connectorExpression, so again not a full information
so then translatedExpression cannot supersede remainingDecomposedPredicate

i think SqlToConnectorExpressionTranslator should work like a DomainTranslator, i..e it should spit out (a) translated result and (b) remaining expression

@martint do you have a suggestion how to handle this for now, so that SqlToConnectorExpressionTranslator change can be a follow up?

[martint]

is lossy.

That should not be the case. The translation should either succeed and be equivalent to the original expression, or fail and return Optional.empty().

[findepi]

Indeed @martint it's not a problem, as it's not lossy.
Although it should be. If a conjunct cannot be translated, we still should push down the others that can. cc @hashhar
But that's something we can follow up on.

[findepi]

Squashed fixups. Previous CI run https://github.com/trinodb/trino/actions/runs/1909288734 has some failures but apparently not related. let's see CI this time.

[findepi]

Merged, thanks!

[slhmy]

Really excieted to see this PR merged.

[findepi]

@martint Shouldn't this use LikeFunctions#LIKE_PATTERN_FUNCTION_NAME, so that we don't have to re-run DesugarLike?

cc @wendigo