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

Use SetOpFactory in rules containing Union operator

    Details

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

      Description

      Union related rules match on LogicalUnion and use this implementation to create the new Union operators. As it is done for other rules, a Union factory can be created and used by them, so subclasses that extend Union can use the rules.

        Activity

        Hide
        vladimirsitnikov Vladimir Sitnikov added a comment -

        I thought the point of using Logical* was to stick with a single implementation and reuse rules.

        What is the use case?

        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - I thought the point of using Logical* was to stick with a single implementation and reuse rules. What is the use case?
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        The default factory creates LogicalUnion operators indeed; but now we can provide a factory that creates our own kind of operators that extend the base Union class. The matching will be based on the Union base class.

        A given use case can be found in Hive. There is a HiveUnion operator that extends Union. I would like to use the UnionMerge rule in Hive. Without this extension, the only solution is to extend the rule and rewrite the match (and possibly transform) methods. Instead, now we will match on base class and providing the UnionFactory to create HiveUnion operators, as other rules already do.

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - The default factory creates LogicalUnion operators indeed; but now we can provide a factory that creates our own kind of operators that extend the base Union class. The matching will be based on the Union base class. A given use case can be found in Hive. There is a HiveUnion operator that extends Union . I would like to use the UnionMerge rule in Hive. Without this extension, the only solution is to extend the rule and rewrite the match (and possibly transform ) methods. Instead, now we will match on base class and providing the UnionFactory to create HiveUnion operators, as other rules already do.
        Hide
        vladimirsitnikov Vladimir Sitnikov added a comment - - edited

        Jesus Camacho Rodriguez, can you please clarify who creates HiveUnion in the first place?
        Can't you perform basic optimizations in Logical* relations?

        HiveUnion operator that extends Union. I would like to use the UnionMerge rule in Hive. Without this extension, the only solution is to extend the rule and rewrite the match (and possibly transform

        I do not follow you. UnionMergeRule operates on LogicalUnions. Eventually (I guess) those LogicalUnions lower to HiveUnion.
        UnionMergeRule will not fire for HiveUnion unless your HiveUnion extends LogicalUnion (however I see little sense in it).

        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - - edited Jesus Camacho Rodriguez , can you please clarify who creates HiveUnion in the first place? Can't you perform basic optimizations in Logical* relations? HiveUnion operator that extends Union. I would like to use the UnionMerge rule in Hive. Without this extension, the only solution is to extend the rule and rewrite the match (and possibly transform I do not follow you. UnionMergeRule operates on LogicalUnions. Eventually (I guess) those LogicalUnions lower to HiveUnion. UnionMergeRule will not fire for HiveUnion unless your HiveUnion extends LogicalUnion (however I see little sense in it).
        Hide
        vladimirsitnikov Vladimir Sitnikov added a comment -

        Using "rule for arbitrary subclass" is a sloppy way.
        Consider this: SemiJoin extends Join.
        In fact, not all the rules that work for Join will work for SemiJoin (there is explicit "if not instanceof SemiJoin" test).
        The same might apply for other nodes.

        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - Using "rule for arbitrary subclass" is a sloppy way. Consider this: SemiJoin extends Join. In fact, not all the rules that work for Join will work for SemiJoin (there is explicit "if not instanceof SemiJoin" test). The same might apply for other nodes.
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        HiveUnion is created by the Hive planner. Logical* operators cannot be extended, so my understanding is that if you want to have your own operator, you may extend to core classes and create e.g. HiveUnion.

        UnionMergeRule operates on LogicalUnions. Eventually (I guess) those LogicalUnions lower to HiveUnion.

        Using "rule for arbitrary subclass" is a sloppy way.
        Consider this: SemiJoin extends Join.
        In fact, not all the rules that work for Join will work for SemiJoin (there is explicit "if not instanceof SemiJoin" test).
        The same might apply for other nodes.

        I think I get your point, we could give the class to match as a parameter too? That makes sense, I have just checked and it is done for other rules in Calcite. By default, the rules would operate on LogicalUnion.

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - HiveUnion is created by the Hive planner. Logical* operators cannot be extended, so my understanding is that if you want to have your own operator, you may extend to core classes and create e.g. HiveUnion . UnionMergeRule operates on LogicalUnions. Eventually (I guess) those LogicalUnions lower to HiveUnion. Using "rule for arbitrary subclass" is a sloppy way. Consider this: SemiJoin extends Join. In fact, not all the rules that work for Join will work for SemiJoin (there is explicit "if not instanceof SemiJoin" test). The same might apply for other nodes. I think I get your point, we could give the class to match as a parameter too? That makes sense, I have just checked and it is done for other rules in Calcite. By default, the rules would operate on LogicalUnion.
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/1ae40b00 .
        Hide
        julianhyde Julian Hyde added a comment -

        Closing now that 1.1.0-incubating has been released.

        Show
        julianhyde Julian Hyde added a comment - Closing now that 1.1.0-incubating has been released.

          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