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

Metadata distribution is broken due to last refactor

    Details

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

      Description

      Metadata distribution is broken due to last refactor.

      Calls of RelMdDistribution will get:

      java.lang.AssertionError: are your methods named wrong?
      

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        If you've created your own metadata provider you will need to add an extra RelMetadataQuery mq argument to each method, per CALCITE-794. This was mentioned in the release notes, http://calcite.apache.org/docs/history.html#v1-6-0.

        Show
        julianhyde Julian Hyde added a comment - If you've created your own metadata provider you will need to add an extra RelMetadataQuery mq argument to each method, per CALCITE-794 . This was mentioned in the release notes, http://calcite.apache.org/docs/history.html#v1-6-0 .
        Hide
        tedxu Ted Xu added a comment -

        I created a pull request at https://github.com/apache/calcite/pull/193.
        Julian please kindly review it.

        Show
        tedxu Ted Xu added a comment - I created a pull request at https://github.com/apache/calcite/pull/193 . Julian please kindly review it.
        Hide
        tedxu Ted Xu added a comment -

        Julian Hyde thanks for looking into this.

        RelDistribute can be a pluggable metadata, however, it is used in Logical RelNodes like LogicalFilter, which is deeply coupled with multiple rules and utilities. It is highly recommended that we fix this bug in calcite-core.

        Show
        tedxu Ted Xu added a comment - Julian Hyde thanks for looking into this. RelDistribute can be a pluggable metadata, however, it is used in Logical RelNodes like LogicalFilter , which is deeply coupled with multiple rules and utilities. It is highly recommended that we fix this bug in calcite-core.
        Hide
        julianhyde Julian Hyde added a comment -

        Ah, I see now. The other way of way of stating this bug is: RelMdDistribution is not tested. If there were a test, the assert would have shown up. Do you feel like adding a test to RelMetadataTest? (I don't think the test in your patch exercises the metadata provider, so it won't exercise this bug.)

        Show
        julianhyde Julian Hyde added a comment - Ah, I see now. The other way of way of stating this bug is: RelMdDistribution is not tested. If there were a test, the assert would have shown up. Do you feel like adding a test to RelMetadataTest? (I don't think the test in your patch exercises the metadata provider, so it won't exercise this bug.)
        Hide
        tedxu Ted Xu added a comment -

        Thanks Julian Hyde, that's exactly what I'm expecting, but I'm not sure if adding RelMdDistribution into default metadata provider could have other impact.

        I'm added RelMdDistribution into default metadata provider, the updated pull request is https://github.com/apache/calcite/pull/194. We can also add RelMdDistribution into RelMetadataTest only, if you feel the former way is too intrusive.

        Show
        tedxu Ted Xu added a comment - Thanks Julian Hyde , that's exactly what I'm expecting, but I'm not sure if adding RelMdDistribution into default metadata provider could have other impact. I'm added RelMdDistribution into default metadata provider, the updated pull request is https://github.com/apache/calcite/pull/194 . We can also add RelMdDistribution into RelMetadataTest only, if you feel the former way is too intrusive.
        Hide
        julianhyde Julian Hyde added a comment -

        I think it's fine to add RelMdDistribution to DefaultRelMetadataProvider.

        I'd also like to add a test or two to RelMetadataTest. Neither of your branches seem to have modified RelMetadataTest; is that what you intended?

        Show
        julianhyde Julian Hyde added a comment - I think it's fine to add RelMdDistribution to DefaultRelMetadataProvider. I'd also like to add a test or two to RelMetadataTest. Neither of your branches seem to have modified RelMetadataTest; is that what you intended?
        Hide
        tedxu Ted Xu added a comment -

        Thanks Julian Hyde.

        The pull request is updated.

        Show
        tedxu Ted Xu added a comment - Thanks Julian Hyde . The pull request is updated.
        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/4dfd6dbb . Thanks for the PR, Ted Xu !
        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.7.0 (2016-03-22).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.7.0 (2016-03-22).

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development