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

Add rule, AggregateValuesRule, that applies to Aggregate on empty relation

    Details

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

      Description

      Add rule, AggegateValuesRule, that applies to Aggregate on empty relation. In CALCITE-1488, Gian Merlino wrote:

      I still have a problem with Aggregates that lack a group set, like SELECT COUNT(*) FROM s.foo WHERE 1 = 0. PruneEmptyRules doesn't reduce that, since the result is going to be one row, not empty. The ValuesReduceRules don't either, because they don't touch Aggregates.

      I ended up addressing this with a rule on my end: https://gist.github.com/gianm/1c192a47a7ce284be8af986f97e6dd8f

      Does that approach make sense? If so do you think it makes sense to contribute to Calcite?
      One thing I'm not sure about is if there's a better way to figure out what should be 0 and what should be NULL other than hard-coding COUNT and SUM0.

      Let's call it AggregateValuesRule, consistent with the naming convention where we start with the type of rel(s) being acted on. Initially it would apply to an empty Values, but potentially it could apply to one or more values.

      I don't recall aggregates having a way to tell us what they will return on empty (or constant) input. In future we could use the constant reducer for that, but for now, the rule should match a particular set of aggregates: COUNT and SUM0 return zero; MIN, MAX, SUM return null. It must abort if it sees an aggregate it does not know how to handle.

        Issue Links

          Activity

          Hide
          vgarg Vineet Garg added a comment -

          Hi Julian,

          Could you elaborate a little more on what this rule will do and why is this required ?

          Show
          vgarg Vineet Garg added a comment - Hi Julian, Could you elaborate a little more on what this rule will do and why is this required ?
          Hide
          julianhyde Julian Hyde added a comment -

          Sorry, I forgot to include the preceding discussion from CALCITE-1488. I just added it to the description. Hopefully it makes sense now.

          Show
          julianhyde Julian Hyde added a comment - Sorry, I forgot to include the preceding discussion from CALCITE-1488 . I just added it to the description. Hopefully it makes sense now.
          Hide
          vgarg Vineet Garg added a comment -

          Thanks Julian Hyde it makes sense now

          Show
          vgarg Vineet Garg added a comment - Thanks Julian Hyde it makes sense now
          Hide
          gian Gian Merlino added a comment -

          Patch in: https://github.com/apache/calcite/pull/324

          I applied this to my local fork too, and it's working for me.

          Show
          gian Gian Merlino added a comment - Patch in: https://github.com/apache/calcite/pull/324 I applied this to my local fork too, and it's working for me.
          Hide
          gian Gian Merlino added a comment -

          Julian Hyde do you think this rule should replace PruneEmptyRules.AGGREGATE_INSTANCE? In that we get rid of that one, and make AggregateValuesRule handle both empty and nonempty groupSets.

          Show
          gian Gian Merlino added a comment - Julian Hyde do you think this rule should replace PruneEmptyRules.AGGREGATE_INSTANCE? In that we get rid of that one, and make AggregateValuesRule handle both empty and nonempty groupSets.
          Hide
          julianhyde Julian Hyde added a comment -

          I think we should keep both.

          • The grand-total case returns 1 row, which is the total of 0 rows. It performs something akin to constant reduction. Handled by the new AggregateValuesRule.
          • The non-grand-total case returns 0 rows and therefore truly "prunes" the tree. Handled by the existing PruneEmptyRules#AGGREGATE_INSTANCE.
          Show
          julianhyde Julian Hyde added a comment - I think we should keep both. The grand-total case returns 1 row, which is the total of 0 rows. It performs something akin to constant reduction. Handled by the new AggregateValuesRule . The non-grand-total case returns 0 rows and therefore truly "prunes" the tree. Handled by the existing PruneEmptyRules#AGGREGATE_INSTANCE .
          Hide
          gian Gian Merlino added a comment -

          Okay, cool. Then I think the patch in #324 should work.

          Show
          gian Gian Merlino added a comment - Okay, cool. Then I think the patch in #324 should work.
          Hide
          julianhyde Julian Hyde added a comment -
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/3f92157d . Thanks for the PR, Gian Merlino !
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.11.0 (2017-01-11).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.11.0 (2017-01-11).

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development