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

Suspicious map.get in VolcanoPlanner.reregister

    Details

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

      Description

      Here is rather strange `map.get` since it never returns a match:
      https://github.com/julianhyde/optiq/blob/ddc6aca8bc76080b5c396a605a0be148aa0a2d79/core/src/main/java/org/eigenbase/relopt/volcano/VolcanoPlanner.java#L1253

      I tried to convert it to `equivRel = map.get(Pair.of(rel.getDigest(), rel.getRowType()))`, however it never manages to enter into `if (equivRel != null && equivRel != rel)` (typically, `equivRel == rel`)

      Can you please double-check if this mapDigestToRel is required here?

      ```java
      Map<Pair<String, RelDataType>, RelNode> mapDigestToRel
      ...
      // Is there an equivalent relational expression? (This might have
      // just occurred because the relational expression's child was just
      // found to be equivalent to another set.)
      RelNode equivRel = mapDigestToRel.get(rel.getDigest());
      if ((equivRel != null) && (equivRel != rel)) {
      ```

      ---------------- Imported from GitHub ----------------
      Url: https://github.com/julianhyde/optiq/issues/205
      Created by: vlsi
      Labels: bug,
      Created at: Tue Mar 25 07:44:34 CET 2014
      State: closed

        Activity

        Hide
        github-import GitHub Import added a comment -

        [Date: Tue Mar 25 22:59:37 CET 2014, Author: julianhyde]

        Yes, that's a bug. Caused by me, when I changed the key from `String` to `Pair<String, RelDataType>` in https://github.com/julianhyde/optiq/commit/e5b0d9d68b802f12aeaaef32ce7ef6a274a2ebda but forgot to change that occurrence. That condition won't be hit that often – set-merges are rare – but it's important that it works. Might be interesting to see whether the test suite was hitting that condition immediately before my commit. Then change the code to make sure that the condition gets hit again.

        Show
        github-import GitHub Import added a comment - [Date: Tue Mar 25 22:59:37 CET 2014, Author: julianhyde ] Yes, that's a bug. Caused by me, when I changed the key from `String` to `Pair<String, RelDataType>` in https://github.com/julianhyde/optiq/commit/e5b0d9d68b802f12aeaaef32ce7ef6a274a2ebda but forgot to change that occurrence. That condition won't be hit that often – set-merges are rare – but it's important that it works. Might be interesting to see whether the test suite was hitting that condition immediately before my commit. Then change the code to make sure that the condition gets hit again.

          People

          • Assignee:
            Unassigned
            Reporter:
            github-import GitHub Import
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development