Partial projections pushdown

[phd3]

Splitting partial projection extraction from #1720. cc @martint @wagnermarkd

Some notes:

Currently, PushProjectionIntoTableScan is applied only in cases where all expressions are completely convertible to a connector level expressions. The only expressions that satisfy this criteria are simple column references, dereference expressions and constants. Any expression that involves anything more complex will not invoke PushProjectionIntoTableScan. So there is no opportunity for pushing down such expressions into the connector.

For example, for projections [“a.b.c”, “d”], the PushProjectionIntoTableScan rule will be invoked. But, for projections [“a.b.c” + “c.d”, “e”], it will be a no-op. This is very primitive, because such a pushdown will not work for even slightly complex projections. In such cases, we know that even if “a.b.c” + “c.d” cannot be pushed down, the individual subexpressions “a.b.c” and “c.d” can be. We need to implement the partial projection pushdown support for that.

After the partial projection support, the PushProjectionIntoTableScan would look like the following:

[findepi]

(only skimming)

[findepi]

is it possible to test this?

also, cmt title is too long. Maybe just Fix translation of FieldDereference to DereferenceExpression.

[phd3]

@findepi Fixed the message. Added a test in TestConnectorExpressionTranslator. I haven't added literals for the test since the translation for them would rely on how ExpressionAnalyzer and LiteralEncoder.

[findepi]

Capitalize "Mismatch", include list sizes in the error message.

Suggested change

	checkState(newConnectorProjections.size() == projections.size(), "mismatch between input and output projections from the connector");
	checkState(newConnectorProjections.size() == projections.size(), "Mismatch between input and output projections from the connector: expected %s but got %s", projections.size(), newConnectorProjections.size());

(maybe you need to rewrap this)

[findepi]

why not private? please revert

[findepi]

@Override

[martint]

We have a very similar class for this already: ReferenceAwareExpressionNodeInliner

[martint]

I generally try to avoid generic "Util" classes, as they tend to become junk drawers over time. Instead, I'd create dedicated utilities (i.e., PartialTranslator, ExpressionReplacer). See, for example ExpressionNodeInliner, ExpressionExtractor or SortExpressionExtractor.

[martint]

Unnecessary formatting change. This was previously:

        Map<String, ColumnHandle> assignments = tableScan.getAssignments()
                .entrySet().stream()

[phd3]

@martint Thanks for the review, I've addressed your comments.

[findepi]

@phd3 please see Travis.

[phd3]

thanks @findepi, I've fixed related errors.

[martint]

One minor comment, but otherwise, it looks good. Let me know once you've addressed it and I'll merge it. Thanks for working on this!

[martint]

Rephrase the commit message: "Update constructor visibility in ReferenceAwareExpressionNodeInliner"

[phd3]

@martint Modified the commit message and cleanly rebased.

[phd3]

@martint There's one non-trivial change for catering to lambda expressions that I've made after the rebase. All tests seem to pass now. Curious if there are other cases of context-dependent symbols like in lambda expressions.

[martint]

There's one non-trivial change for catering to lambda expressions that I've made after the rebase. All tests seem to pass now. Curious if there are other cases of context-dependent symbols like in lambda expressions.

The only other case would be in the context of a correlated subquery. But those appear at the "top level" from the point of view of the plan operation (i.e., not within a nested context like lambda expressions), are are not bound to columns.

[martint]

Merged, thanks for working on this!