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

ValuesReduceRule should ignore empty Values

    Details

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

      Description

      ValuesReduceRule doesn't propagate rowType on Project of empty Values. If ValuesReduceRule is trying to reduce a Project on top of an empty Values, then changeCount ends up 0, and it returns the underlying Values. This leads to an assertion failure because the returned Values does not have the expected rowType.

      One way to fix this is changing the "Filter had no effect" logic from "changeCount == 0" to "changeCount == 0 && projectExprs == null".

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        Gian Merlino, Your analysis of the problem is correct, and your proposed fix would have worked. However, since we already have rules for "propagating emptiness" (in PruneEmptyRules), the simplest thing is to have ValuesReduceRule back off and let those rules do their job. (After all, if you have a filter or project on top of an empty relation, it's a waste of effort to simplify the filter or project, because you know the result is going to be empty.) So that's what I have done.

        Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/f36584ab.

        You will need to enable PruneEmptyRules.PROJECT_INSTANCE, if you haven't already.

        Show
        julianhyde Julian Hyde added a comment - Gian Merlino , Your analysis of the problem is correct, and your proposed fix would have worked. However, since we already have rules for "propagating emptiness" (in PruneEmptyRules ), the simplest thing is to have ValuesReduceRule back off and let those rules do their job. (After all, if you have a filter or project on top of an empty relation, it's a waste of effort to simplify the filter or project, because you know the result is going to be empty.) So that's what I have done. Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/f36584ab . You will need to enable PruneEmptyRules.PROJECT_INSTANCE , if you haven't already.
        Hide
        gian Gian Merlino added a comment - - edited

        Thanks Julian Hyde! I applied this patch locally and it works for me.

        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.

        Show
        gian Gian Merlino added a comment - - edited Thanks Julian Hyde ! I applied this patch locally and it works for me. 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.
        Hide
        julianhyde Julian Hyde added a comment -

        Yes, that rule would be useful to us. Thanks for offering. I have logged CALCITE-1489 and answered some of your questions there.

        Show
        julianhyde Julian Hyde added a comment - Yes, that rule would be useful to us. Thanks for offering. I have logged CALCITE-1489 and answered some of your questions there.
        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:
            gian Gian Merlino
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development