[BEAM-8508] [SQL] Standalone filter push down #9943

11moon11 · 2019-10-30T23:09:01Z

Update the push-down rule to perform predicate push-down for IOs that do not support project push-down.
Update the BeamCalcRel#beamComputeSelfCost method to favor programs with smaller predicates.
Fixed a bug when selecting a field more than once to not drop the calc, since IOs cannot duplicate projected fields. Also added test cases to ensure expected behavior from Project, Filter, ProjectAndFilter push-downs.
PushDownRule should not be applied to IO Rels more than once.
Create a BeamPushDownIOSourceRel.

Other changes:

Rename TestTableProviderWithFilterPushDown to TestTableProviderWithFilterAndProjectPushDown, since it tests when both are supported.
Created a new file TestTableProviderWithFilterPushDown to test when only filter push-down is supported.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
R: @apilloud
cc: @amaliujia
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Gearpump	Samza	Spark
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

apilloud

Two related concerns, otherwise LGTM.

apilloud · 2019-10-31T17:44:21Z

...l/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/test/TestTableProvider.java

-      // When project push-down is supported.
-      if ((options == PushDownOptions.PROJECT || options == PushDownOptions.BOTH)
-          && !fieldNames.isEmpty()) {
+      // When project push-down is supported or field reordering is needed.


This is interesting. I wouldn't expect an IO to support field reordering unless it also supports push-down. Can you explain what is going on here?

The problem I was trying to fix by modifying that if statement is a scenario when an IO only supports Filter push-down and predicate can be completely pushed-down to IO layer, all fields are selected, so there is no need to preserve the Calc to perform project, but selected fields are not in the random order.
In the scenario described above an IO does need to reorder fields (either with a Select or by not dropping a Calc).

I agree that the current if statement is incorrect and it should look more like what it used to, but with an additional check. Not quite sure how to check that the IORel is not followed by a CalcRel and that the fields are not selected in the order they are present in the original schema, but I'm working on it.

Decided to not drop the Calc when fields are selected in a different order for now.

apilloud · 2019-10-31T17:46:16Z

...sions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rule/BeamIOPushDownRule.java

+    if (isProjectRenameOnlyProgram(program)
+        && tableFilter.getNotSupported().isEmpty()
+        && (beamSqlTable.supportsProjects()
+            || calc.getRowType().getFieldCount() == calcInputRowType.getFieldCount())) {


I think this needs to verify the order as well as the count.

I decided not to drop the Calc when project push-down is not supported.
It might make sense to allow IOs communicate to the Rule that they support field reordering to decided whether a Calc should be dropped.

Still working on checking the order, do not merge yet.

Moved this check into a separate method to keep things readable.
Updated the check to compare a list of projected field names to the list passed to a Calc as input.

11moon11 · 2019-11-05T00:58:39Z

...sions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rule/BeamIOPushDownRule.java

-    if (isProjectRenameOnlyProgram(program) && tableFilter.getNotSupported().isEmpty()) {
+    //    And
+    // 3. And IO supports project push-down OR all fields are projected by a Calc.
+    if (isProjectRenameOnlyProgram(program, beamSqlTable.supportsProjects())


Another case when Calc should not be dropped is when an IO does not support field reordering and fields are not projected in the same order they are listed in the IO Schema.
Did not get cough by tests, because TestTable providers uses Select, which does field reordering for us.

This should be resolved now, IOs can specify whether they support field reordering. When reordering in not supported and fields are projected in a different order (from Schema) - Calc should not be dropped.

…ost model for BeamCalcRel.

…roject push-down is not supported

...l/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BigQueryTable.java

11moon11 · 2019-11-06T23:35:14Z

PushDownRule should not be applied to the same BeamIOSourceRel more than once.

11moon11 · 2019-11-07T19:56:21Z

...l/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BigQueryTable.java

@@ -138,12 +138,7 @@ public BeamTableStatistics getTableStatistics(PipelineOptions options) {
      builder.withSelectedFields(fieldNames);


Should be:
builder = builder.withSelectedFields(fieldNames);
Will fix in a different PR.

I think this is fine as it is. Builder normally stores the changes and just returns itself as a convenience.

Talked about this more, builder isn't actually a builder.

apilloud · 2019-11-07T23:01:06Z

...s/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamPushDownIOSourceRel.java

+  @Override
+  public BeamCostModel beamComputeSelfCost(RelOptPlanner planner, RelMetadataQuery mq) {
+    return super.beamComputeSelfCost(planner, mq)
+        .multiplyBy((double) 1 / (getRowType().getFieldCount() + 1));


This seems fine for now if tests pass, but this has a very outsized impact on the cost. I think the impact needs to be less than 1.0, such that two IOs with different row counts still have the correct relative cost.

apilloud

LGTM

11moon11 changed the title ~~[BEAM-8508] [SQL] [WIP] Standalone filter push down~~ [BEAM-8508] [SQL] Standalone filter push down Oct 30, 2019

apilloud self-requested a review October 31, 2019 17:38

apilloud approved these changes Oct 31, 2019

View reviewed changes

11moon11 force-pushed the StandaloneFilterPushDown branch from ef60183 to e70aa72 Compare November 1, 2019 17:09

11moon11 commented Nov 5, 2019

View reviewed changes

11moon11 force-pushed the StandaloneFilterPushDown branch from 1b23074 to 7586096 Compare November 5, 2019 22:38

11moon11 added 3 commits November 5, 2019 14:46

Filter push-down should not be reliant on project push-down [BEAM-8508]

5ff7428

Fixed a bug with selecting the same field more than once. Changed a c…

d9e4134

…ost model for BeamCalcRel.

Selecting fields in a different order should not drop the Calc when p…

599bd21

…roject push-down is not supported

11moon11 force-pushed the StandaloneFilterPushDown branch from dd6cfbc to 7586096 Compare November 5, 2019 22:46

11moon11 added 2 commits November 5, 2019 14:47

Refactoring

08381fd

spotlesApply

e79bcf8

11moon11 force-pushed the StandaloneFilterPushDown branch from 7586096 to e79bcf8 Compare November 5, 2019 22:48

11moon11 commented Nov 6, 2019

View reviewed changes

...l/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BigQueryTable.java Outdated Show resolved Hide resolved

11moon11 requested a review from apilloud November 6, 2019 18:24

IOPushDownRule should not be applied more than once to the same IORels

987148a

11moon11 force-pushed the StandaloneFilterPushDown branch from dcfdb8a to 987148a Compare November 7, 2019 19:31

11moon11 commented Nov 7, 2019

View reviewed changes

11moon11 mentioned this pull request Nov 7, 2019

[BEAM-8583] [SQL] Big query filter push down #10030

Merged

3 tasks

apilloud reviewed Nov 7, 2019

View reviewed changes

apilloud approved these changes Nov 7, 2019

View reviewed changes

apilloud merged commit a48662d into apache:master Nov 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-8508] [SQL] Standalone filter push down #9943

[BEAM-8508] [SQL] Standalone filter push down #9943

11moon11 commented Oct 30, 2019 •

edited

apilloud left a comment

apilloud Oct 31, 2019

11moon11 Oct 31, 2019

11moon11 Oct 31, 2019

apilloud Oct 31, 2019

11moon11 Oct 31, 2019

11moon11 Nov 5, 2019

11moon11 Nov 5, 2019

11moon11 Nov 5, 2019

11moon11 Nov 5, 2019

11moon11 commented Nov 6, 2019

11moon11 Nov 7, 2019

apilloud Nov 7, 2019

apilloud Nov 7, 2019

apilloud Nov 7, 2019

apilloud left a comment

		@@ -138,12 +138,7 @@ public BeamTableStatistics getTableStatistics(PipelineOptions options) {
		builder.withSelectedFields(fieldNames);

[BEAM-8508] [SQL] Standalone filter push down #9943

[BEAM-8508] [SQL] Standalone filter push down #9943

Conversation

11moon11 commented Oct 30, 2019 • edited

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

apilloud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

11moon11 commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apilloud left a comment

Choose a reason for hiding this comment

11moon11 commented Oct 30, 2019 •

edited