[WIP] Return proper unenforced constraint for base-jdbc connector

[Praveen2112]

Since all the jdbc connector enforce all of the domains that are passed as a constraint, we can return the unenforced constraint as TupleDomain.all() so that Presto need not apply those filter criteria once again.

[findepi]

Since all the jdbc connector enforce all of the domains that are passed as a constraint

This isn't a correct assumption.

1. there is com.facebook.presto.plugin.jdbc.QueryBuilder#isAcceptedType, so some constraints are not being pushed down
2. this will become more flexible in #12151

[Praveen2112]

@findepi . Thanks for the review. I have updated it. Now we create a TupleDomain which has the constraints which is not being pushed down. Can also be moved to the model as proposed in #12151

[kokosing]

Still no tests.

[kokosing]

This should be something that every jdbc connector could override.

[Praveen2112]

So can we push it to BaseJdbcClient so that the jdbc connector can override it.

[kokosing]

@findepi what do you think about this location?

[findepi]

@Praveen2112 the PR i linked before (#12151) refactors the code base around pushdown and already delegates the responsibility to the BaseJdbcClient. I recommend that you base your work on that PR.

[Praveen2112]

@findepi isAcceptedType in present in QueryBuilder as a private static method can we move it to some Util class so that it could be accessed by JdbcMetadata and QueryBuilder.

[findepi]

@Praveen2112 do you mean the code in #12151 ?

[Praveen2112]

@findepi yes

[findepi]

good. there -- com.facebook.presto.plugin.jdbc.QueryBuilder#isAcceptedType is now an impl detail of QueryBuilder and has no logic.
com.facebook.presto.plugin.jdbc.ColumnMapping#isPredicatePushDownAllowed is where the information now lives

[Praveen2112]

@findepi Okay so If I am not wrong we would fetch the ColumnMapping from the above API in JdbcClient#toPrestoType and use it to do the constraint check.
The code will be something like this

Optional<ColumnMapping> mapping = client.toPrestoType(session, column.getJdbcTypeHandle())
if (mapping.isPresent() && mapping.get().isPredicatePushDownAllowed()) {
    return null;
}

[electrum]

I am doubtful this is a worthwhile change. We are trading what is likely minimal extra CPU usage for complexity and potential correctness bugs. Have you run any benchmarks to determine if this has an impact for real queries?