Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-5524

Support early out for code generated conjunctive conditions

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.2.0, 1.1.4, 1.3.0
    • Fix Version/s: 1.3.0
    • Component/s: Table API & SQL
    • Labels:
      None

      Description

      Currently, all nested conditions for a conjunctive predicate are evaluated before the conjunction is checked.

      A condition like (v1 == v2) && (v3 < 5) would be compiled into

      boolean res1;
      if (v1 == v2) {
        res1 = true;
      } else {
        res1 = false;
      }
      
      boolean res2;
      if (v3 < 5) {
        res2 = true;
      } else {
        res2 = false;
      }
      
      boolean res3;
      if (res1 && res2) {
        res3 = true;
      } else {
        res3 = false;
      }
      
      if (res3) {
        // emit something
      }
      

      It would be better to leave the generated code as early as possible, e.g., with a return instead of res1 = false. The code generator needs a bit of context information for that.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user KurtYoung opened a pull request:

          https://github.com/apache/flink/pull/3372

          FLINK-5524 [table] Support early out for code generated AND/OR condition

          For condition like a AND b, if the result of a is false, we can save b from execution.

          For condition like a OR b, if the result of a is true, we can also save b from execution.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/KurtYoung/flink flink-5524

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3372.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #3372


          commit 4f8c07ffc6ac99da67803114edb732e20df793e6
          Author: Kurt Young <ykt836@gmail.com>
          Date: 2017-02-21T06:35:17Z

          FLINK-5524 [table] Support early out for code generated AND/OR conditions


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user KurtYoung opened a pull request: https://github.com/apache/flink/pull/3372 FLINK-5524 [table] Support early out for code generated AND/OR condition For condition like a AND b, if the result of a is false, we can save b from execution. For condition like a OR b, if the result of a is true, we can also save b from execution. You can merge this pull request into a Git repository by running: $ git pull https://github.com/KurtYoung/flink flink-5524 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3372.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3372 commit 4f8c07ffc6ac99da67803114edb732e20df793e6 Author: Kurt Young <ykt836@gmail.com> Date: 2017-02-21T06:35:17Z FLINK-5524 [table] Support early out for code generated AND/OR conditions
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twalthr commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3372#discussion_r102185303

          — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/api/scala/batch/table/CalcITCase.scala —
          @@ -217,6 +218,65 @@ class CalcITCase(
          }

          @Test
          + def testDisjunctivePredicateEarlyOut(): Unit = {
          — End diff –

          Can you convert this test to a unit test with `ExpressionTestBase`? You can add them to `ScalarOperatorsTest`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user twalthr commented on a diff in the pull request: https://github.com/apache/flink/pull/3372#discussion_r102185303 — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/api/scala/batch/table/CalcITCase.scala — @@ -217,6 +218,65 @@ class CalcITCase( } @Test + def testDisjunctivePredicateEarlyOut(): Unit = { — End diff – Can you convert this test to a unit test with `ExpressionTestBase`? You can add them to `ScalarOperatorsTest`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user KurtYoung commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3372#discussion_r102361046

          — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/api/scala/batch/table/CalcITCase.scala —
          @@ -217,6 +218,65 @@ class CalcITCase(
          }

          @Test
          + def testDisjunctivePredicateEarlyOut(): Unit = {
          — End diff –

          Sure, thanks for the hint.

          Show
          githubbot ASF GitHub Bot added a comment - Github user KurtYoung commented on a diff in the pull request: https://github.com/apache/flink/pull/3372#discussion_r102361046 — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/api/scala/batch/table/CalcITCase.scala — @@ -217,6 +218,65 @@ class CalcITCase( } @Test + def testDisjunctivePredicateEarlyOut(): Unit = { — End diff – Sure, thanks for the hint.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twalthr commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3372#discussion_r102407744

          — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/api/scala/batch/table/CalcITCase.scala —
          @@ -217,6 +217,21 @@ class CalcITCase(
          }

          @Test
          + def testConjunctivePredicate(): Unit = {
          — End diff –

          This is basically a pure code generation change, we really need no ITCase for that. Can you remove this one? We really should keep ITCases to a minimum. They add to much overhead.

          Show
          githubbot ASF GitHub Bot added a comment - Github user twalthr commented on a diff in the pull request: https://github.com/apache/flink/pull/3372#discussion_r102407744 — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/api/scala/batch/table/CalcITCase.scala — @@ -217,6 +217,21 @@ class CalcITCase( } @Test + def testConjunctivePredicate(): Unit = { — End diff – This is basically a pure code generation change, we really need no ITCase for that. Can you remove this one? We really should keep ITCases to a minimum. They add to much overhead.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user KurtYoung commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3372#discussion_r102421601

          — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/api/scala/batch/table/CalcITCase.scala —
          @@ -217,6 +217,21 @@ class CalcITCase(
          }

          @Test
          + def testConjunctivePredicate(): Unit = {
          — End diff –

          ok, will do.

          Show
          githubbot ASF GitHub Bot added a comment - Github user KurtYoung commented on a diff in the pull request: https://github.com/apache/flink/pull/3372#discussion_r102421601 — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/api/scala/batch/table/CalcITCase.scala — @@ -217,6 +217,21 @@ class CalcITCase( } @Test + def testConjunctivePredicate(): Unit = { — End diff – ok, will do.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user KurtYoung commented on the issue:

          https://github.com/apache/flink/pull/3372

          hey @twalthr, any more comments or this can be merged

          Show
          githubbot ASF GitHub Bot added a comment - Github user KurtYoung commented on the issue: https://github.com/apache/flink/pull/3372 hey @twalthr, any more comments or this can be merged
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twalthr commented on the issue:

          https://github.com/apache/flink/pull/3372

          Thanks for the update @KurtYoung. The code looks good. I will merge this.

          Show
          githubbot ASF GitHub Bot added a comment - Github user twalthr commented on the issue: https://github.com/apache/flink/pull/3372 Thanks for the update @KurtYoung. The code looks good. I will merge this.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/3372

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3372
          Hide
          twalthr Timo Walther added a comment -

          Fixed in 1.3.0: f6c9b32c1f1fd7521e2dbe16ab20985a45d7cb07

          Show
          twalthr Timo Walther added a comment - Fixed in 1.3.0: f6c9b32c1f1fd7521e2dbe16ab20985a45d7cb07

            People

            • Assignee:
              ykt836 Kurt Young
              Reporter:
              fhueske Fabian Hueske
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development