Add applyFilter equivalent to Constraint-based table layout selection #541
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Mar 26, 2019
martint
force-pushed
the
pushdown-constraint
branch
from
3187f86
to
b4fd791
Compare
kokosing
reviewed
Mar 26, 2019
| @@ -602,4 +602,23 @@ default ConnectorTableProperties getTableProperties(ConnectorSession session, Co | |||
| { | |||
| return Optional.empty(); | |||
| } | |||
|
|
|||
| /** | |||
| * Attempt to push down the provided constraint into the table. This method is provided as an alternative to | |||
There was a problem hiding this comment.
it is not alternative, but a replacement.
| return Optional.of(new ValuesNode(idAllocator.getNextId(), node.getOutputSymbols(), ImmutableList.of())); | ||
| TableHandle newTable; | ||
| TupleDomain<ColumnHandle> remainingFilter; | ||
| if (metadata.usesLegacyTableLayouts(session, node.getTable())) { |
There was a problem hiding this comment.
nit: I would put legacy branch into the else branch.
| * <p> | ||
| * <b>Note</b>: it's critical for connectors to return Optional.empty() if calling this method has no effect for that | ||
| * invocation, even if the connector generally supports pushdown. Doing otherwise can cause the optimizer | ||
| * to loop indefinitely. |
There was a problem hiding this comment.
We could also check if the were no change. For example:
- checking if the remaining predicate is empty
- seeing that remaining predicate is equal to constraint that we are trying to push down.
There was a problem hiding this comment.
I'm not sure that is possible. The connector could be applying the constraint internally by updating its handle, but it may not be able to enforce the change completely. This means that there would be no external change visible, but big improvements should be happening internally.
dain
reviewed
Mar 26, 2019
| @@ -265,7 +265,7 @@ public ConnectorTableLayout getTableLayout(ConnectorSession session, ConnectorTa | |||
| TpchTableLayoutHandle layout = (TpchTableLayoutHandle) handle; | |||
|
|
|||
| // tables in this connector have a single layout | |||
| return getTableLayouts(session, layout.getTable(), Constraint.alwaysTrue(), Optional.empty()) | |||
| return getTableLayouts(session, layout.getTable(), new Constraint<>(layout.getPredicate()), Optional.empty()) | |||
| * <p> | ||
| * <b>Note</b>: it's critical for connectors to return Optional.empty() if calling this method has no effect for that | ||
| * invocation, even if the connector generally supports pushdown. Doing otherwise can cause the optimizer | ||
| * to loop indefinitely. |
There was a problem hiding this comment.
I'm not sure that is possible. The connector could be applying the constraint internally by updating its handle, but it may not be able to enforce the change completely. This means that there would be no external change visible, but big improvements should be happening internally.
kokosing
approved these changes
Mar 27, 2019
getLayout(LayoutHandle) was picking a new predicate and ignoring the previously pushed-down predicate.
martint
force-pushed
the
pushdown-constraint
branch
2 times, most recently
from
c7e6765
to
55824f4
Compare
Current constraint is effectively a cache of the predicate that describes the data the connector will produce. It is unnecessary, as the engine can obtain it by calling getTableProperties and retrieving it directly.
The ability to push down a Constraint (TupleDomain + opaque predicate evaluator) is the last piece needed to fully deprecate Table Layouts. The new applyFilter function introduces a specialized API that connectors can implement to obtain the same behavior without having to deal with Table Layouts, TableLayoutHandle, etc. It is meant as a transitional API until the fully-feature applyFilter with generalized predicates is implemented.
martint
force-pushed
the
pushdown-constraint
branch
from
55824f4
to
21fd923
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The ability to push down a Constraint (TupleDomain + opaque predicate evaluator) is the last piece needed to fully deprecate Table Layouts. The new applyFilter function introduces a specialized API that connectors can implement to obtain the same behavior without having to deal with Table Layouts, TableLayoutHandle, etc. It is meant as a transitional API until the fully-feature applyFilter with generalized predicates is implemented.