Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-1577

Druid adapter: Incorrect result - limit on timestamp disappears

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 1.11.0
    • Fix Version/s: 1.12.0
    • Component/s: druid
    • Labels:
      None

      Description

      This can be observed with the following query:

      SELECT DISTINCT `__time`
      FROM store_sales_sold_time_subset
      ORDER BY `__time` ASC
      LIMIT 10;
      

      Query is translated to Druid timeseries, but limit operator disappears.

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.12.0 (2017-03-24).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.12.0 (2017-03-24).
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/e5c9f2ed .
          Hide
          julianhyde Julian Hyde added a comment -

          Your fix looks good. After I applied on top of my fix to CALCITE-1578 et al, the sort got pulled up out of Druid, so the explain changed, and I was able to execute the query without Druid blowing up. So I added .returnsOrdered to the test cases; that should give us more confidence if we need to maintain the test in future. Will commit shortly, when the suite passes.

          Show
          julianhyde Julian Hyde added a comment - Your fix looks good. After I applied on top of my fix to CALCITE-1578 et al, the sort got pulled up out of Druid, so the explain changed, and I was able to execute the query without Druid blowing up. So I added .returnsOrdered to the test cases; that should give us more confidence if we need to maintain the test in future. Will commit shortly, when the suite passes.
          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Sure, I have created https://github.com/apache/calcite/pull/355/commits/66339365bd40119d7bba5fa3ba42e5bbac6835d0 .
          Hide
          julianhyde Julian Hyde added a comment -

          Can you create a PR in https://github.com/apache/calcite/pulls and close the one in https://github.com/julianhyde/calcite/pulls (I don't believe that it's a valid "contribution" unless the PR is to Apache, so I would not be able to commit, you would have to.)

          I don't see your PR for CALCITE-1578, but in any case, I am just about to check in a commit which fixes CALCITE-1578, CALCITE-1579 and CALCITE-1580, and another commit which fixes CALCITE-1587. I do not have time to fix CALCITE-1591 just now.

          I'll review your fix shortly.

          Show
          julianhyde Julian Hyde added a comment - Can you create a PR in https://github.com/apache/calcite/pulls and close the one in https://github.com/julianhyde/calcite/pulls (I don't believe that it's a valid "contribution" unless the PR is to Apache, so I would not be able to commit, you would have to.) I don't see your PR for CALCITE-1578 , but in any case, I am just about to check in a commit which fixes CALCITE-1578 , CALCITE-1579 and CALCITE-1580 , and another commit which fixes CALCITE-1587 . I do not have time to fix CALCITE-1591 just now. I'll review your fix shortly.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Julian Hyde, I have created a fix for this in https://github.com/julianhyde/calcite/pull/9/commits/66339365bd40119d7bba5fa3ba42e5bbac6835d0 . I created a PR against CALCITE-1578 too. Could you review the new code and merge it into your branch?

          The PR contains tests. However, one of the tests does not currently pass, as the Sort operation is not pushed into Druid. I double-checked and logic is correct, hence I think it has to do with the optimizer and the cost of the expression.

          For those new tests in the PR, which are transformed into timeseries queries, I do not run the query against Druid: in my environment, Druid crashes (probably not enough memory to execute timeseries + sort).

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , I have created a fix for this in https://github.com/julianhyde/calcite/pull/9/commits/66339365bd40119d7bba5fa3ba42e5bbac6835d0 . I created a PR against CALCITE-1578 too. Could you review the new code and merge it into your branch? The PR contains tests. However, one of the tests does not currently pass, as the Sort operation is not pushed into Druid. I double-checked and logic is correct, hence I think it has to do with the optimizer and the cost of the expression. For those new tests in the PR, which are transformed into timeseries queries, I do not run the query against Druid: in my environment, Druid crashes (probably not enough memory to execute timeseries + sort).

            People

            • Assignee:
              jcamachorodriguez Jesus Camacho Rodriguez
              Reporter:
              jcamachorodriguez Jesus Camacho Rodriguez
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development