Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.9.0
    • Component/s: core
    • Labels:
      None

      Description

      Currently RexCall compute digests eagerly in its constructor, also, it compute digest every time when toString invoked. It may cause performance issue when the RexCall tree is very large.

      1. p.patch
        5 kB
        Julian Hyde

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Is RexCall.toString() called often? (Not counting the case where tracing is enabled.) If it is, we have a performance bug that we should definitely fix.

          If we have large expressions, then there is considerable duplication of strings. For example, in 0 + 1 + 2 + ... + 999999, the string "99998 + 999999" will occur in all digests but one. Maybe a rope would be a better data structure.

          Show
          julianhyde Julian Hyde added a comment - Is RexCall.toString() called often? (Not counting the case where tracing is enabled.) If it is, we have a performance bug that we should definitely fix. If we have large expressions, then there is considerable duplication of strings. For example, in 0 + 1 + 2 + ... + 999999 , the string "99998 + 999999" will occur in all digests but one. Maybe a rope would be a better data structure.
          Hide
          tedxu Ted Xu added a comment -

          Hi Julian Hyde please have a look on my patch https://github.com/apache/calcite/pull/264, thanks!

          Show
          tedxu Ted Xu added a comment - Hi Julian Hyde please have a look on my patch https://github.com/apache/calcite/pull/264 , thanks!
          Hide
          tedxu Ted Xu added a comment -

          Thanks Julian Hyde, rope looks a good solution for your case! I will have a look into that.

          I'm not addressing the duplicated digest problem right now. We may fix the duplicated digest computation first.

          Show
          tedxu Ted Xu added a comment - Thanks Julian Hyde , rope looks a good solution for your case! I will have a look into that. I'm not addressing the duplicated digest problem right now. We may fix the duplicated digest computation first.
          Hide
          julianhyde Julian Hyde added a comment -

          Can you add a stress test (see attached patch).

          Show
          julianhyde Julian Hyde added a comment - Can you add a stress test (see attached patch).
          Hide
          tedxu Ted Xu added a comment -

          Your tests added accordingly.

          Show
          tedxu Ted Xu added a comment - Your tests added accordingly.
          Hide
          julianhyde Julian Hyde added a comment - - edited

          Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/0ce83391. Thanks for the PR, Ted Xu!

          Although ropes are discussed above, this fix does not include ropes. As of this fix, the digest of a call contains, by copying the characters of the constituent strings, the digests of the arguments. It is computed the first time that someone calls RexCall#toString.

          Show
          julianhyde Julian Hyde added a comment - - edited Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/0ce83391 . Thanks for the PR, Ted Xu ! Although ropes are discussed above, this fix does not include ropes. As of this fix, the digest of a call contains, by copying the characters of the constituent strings, the digests of the arguments. It is computed the first time that someone calls RexCall#toString .
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Resolved in release 1.9.0 (2016-09-22)

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.9.0 (2016-09-22)

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              tedxu Ted Xu
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development