Derby
  1. Derby
  2. DERBY-4608

Unnecessary conversion of binary values to strings in SQLBinary.compare()

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.5.3.0
    • Fix Version/s: 10.6.1.0
    • Component/s: SQL
    • Labels:
      None
    • Bug behavior facts:
      Performance

      Description

      SQLBinary.compare(int,DataValueDescriptor,boolean,boolean) converts the values to strings in order to check whether any of them are null. The isNull() method should be used instead to prevent the unnecessary conversion to strings.

      See this thread on derby-user: http://mail-archives.apache.org/mod_mbox/db-derby-user/201003.mbox/%3C001801cad09b$09aef650$1d0ce2f0$@ru%3E

      1. isnull.diff
        10 kB
        Knut Anders Hatlen

        Activity

        Hide
        Knut Anders Hatlen added a comment -

        The attached patch makes SQLBinary.compare() use isNull() instead of getString(). It also changes the HeapScan performance test so that it can run tests with binary strings instead of character strings, and adds a test case that uses predicates. When running the test case with predicates on binary data, the fix in SQLBinary reduces the time per iteration from 30 seconds to 5-6 seconds on my laptop.

        Running the full regression test suite now.

        Show
        Knut Anders Hatlen added a comment - The attached patch makes SQLBinary.compare() use isNull() instead of getString(). It also changes the HeapScan performance test so that it can run tests with binary strings instead of character strings, and adds a test case that uses predicates. When running the test case with predicates on binary data, the fix in SQLBinary reduces the time per iteration from 30 seconds to 5-6 seconds on my laptop. Running the full regression test suite now.
        Hide
        Bryan Pendleton added a comment -

        The patch looks good to me, thanks for tracking this down.

        I don't particularly care for the wording of the THROWASSERT
        in SQLBinary.java just prior to your change, but of course that's
        entirely unrelated, just something that struck my eye while
        reading your patch.

        +1 to your patch, which looks thorough and complete.

        Show
        Bryan Pendleton added a comment - The patch looks good to me, thanks for tracking this down. I don't particularly care for the wording of the THROWASSERT in SQLBinary.java just prior to your change, but of course that's entirely unrelated, just something that struck my eye while reading your patch. +1 to your patch, which looks thorough and complete.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks Bryan! All the regression tests ran cleanly, so I committed the patch with revision 931189.

        The wording of the THROWASSERT struck me too as somewhat too colourful. I'll try to come up with something more neutral and change that too.

        Show
        Knut Anders Hatlen added a comment - Thanks Bryan! All the regression tests ran cleanly, so I committed the patch with revision 931189. The wording of the THROWASSERT struck me too as somewhat too colourful. I'll try to come up with something more neutral and change that too.

          People

          • Assignee:
            Knut Anders Hatlen
            Reporter:
            Knut Anders Hatlen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development