Apache Drill
  1. Apache Drill
  2. DRILL-45

Create Logical Plan builder for programmatic creation of a Logical Plan

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Labels:
      None

      Description

      Will be useful when users are generating a Logical Plan within Java. First consumer would likely be the SQL parser.

      1. drill-45-2.patch
        24 kB
        Ben Becker
      2. DRILL-45.patch
        31 kB
        David Alves
      3. DRILL-45.patch
        25 kB
        Ben Becker

        Activity

        Hide
        David Alves added a comment -

        Here's a first stab at this, https://github.com/apache/incubator-drill/pull/11.patch.

        It's admittedly simple but I'd like to hear your thoughts on it before proceeding.

        Show
        David Alves added a comment - Here's a first stab at this, https://github.com/apache/incubator-drill/pull/11.patch . It's admittedly simple but I'd like to hear your thoughts on it before proceeding.
        Hide
        Jacques Nadeau added a comment -

        I like the approach. Probably would help to be able to add builders of builders. For example, adding a LogicalOperatorBuilder to LogicalPlanBuilder. Julian Hyde, do you have any opinions?

        Show
        Jacques Nadeau added a comment - I like the approach. Probably would help to be able to add builders of builders. For example, adding a LogicalOperatorBuilder to LogicalPlanBuilder. Julian Hyde , do you have any opinions?
        Hide
        David Alves added a comment - - edited

        I agree that we need more builders, but IMO those should not be in the LogicalPlanBuilder (my reasoning is that we wouldn't want to keep updating it to reflect every Logical Op and/or logical Op field).

        My idea is to create builders inside the logical ops themselves (using a builder hierarchy that follow the op hierarchy).

        In the end using them would look approximately like:

        StorageEngine engine = HBaseStorageEngine.builder()
          .url(...)
          .build()
        
        LogicalPlan plan = LogicalPlan.builder()
        .planProperties(PlanProperties.builder()
          .version(1)
          .generator(generator)
          .build())
        .addStorageEngine(engine)
        .addLogicalOperator(Scan.builder()
          .storageEngine(engine)
          .build())
        .build()
        
        

        Along with adding builders to ops we would also make them immutable.

        Show
        David Alves added a comment - - edited I agree that we need more builders, but IMO those should not be in the LogicalPlanBuilder (my reasoning is that we wouldn't want to keep updating it to reflect every Logical Op and/or logical Op field). My idea is to create builders inside the logical ops themselves (using a builder hierarchy that follow the op hierarchy). In the end using them would look approximately like: StorageEngine engine = HBaseStorageEngine.builder() .url(...) .build() LogicalPlan plan = LogicalPlan.builder() .planProperties(PlanProperties.builder() .version(1) .generator(generator) .build()) .addStorageEngine(engine) .addLogicalOperator(Scan.builder() .storageEngine(engine) .build()) .build() Along with adding builders to ops we would also make them immutable.
        Hide
        Jacques Nadeau added a comment -

        This sounds good. The only thing I was saying is sometimes it is nice to have support for .planProperties(PlanPropertiesBuilder b) in addition to .planProperties(PlanProperties p).

        Show
        Jacques Nadeau added a comment - This sounds good. The only thing I was saying is sometimes it is nice to have support for .planProperties(PlanPropertiesBuilder b) in addition to .planProperties(PlanProperties p).
        Hide
        David Alves added a comment -

        adds a builder to plan properties and makes everything immutable after build.

        Operators/Engines would probably benefit from the same treatment but doing this all at once would make the patch unreviewable. Thoughts?

        Show
        David Alves added a comment - adds a builder to plan properties and makes everything immutable after build. Operators/Engines would probably benefit from the same treatment but doing this all at once would make the patch unreviewable. Thoughts?
        Hide
        Jacques Nadeau added a comment -

        Can you rebase off the current master branch?

        Show
        Jacques Nadeau added a comment - Can you rebase off the current master branch?
        Hide
        Jacques Nadeau added a comment -

        Reassigning to Ben to get this rebased and enhance functionality.

        Show
        Jacques Nadeau added a comment - Reassigning to Ben to get this rebased and enhance functionality.
        Hide
        Ben Becker added a comment -

        Attaching drill-45-2.patch, which updates the previous patch to the latest execwork branch. Implements PlanPropertiesBuilder, LogicalPlanBuilder, ScanBuilder and StoreBuilder.

        Show
        Ben Becker added a comment - Attaching drill-45-2.patch, which updates the previous patch to the latest execwork branch. Implements PlanPropertiesBuilder, LogicalPlanBuilder, ScanBuilder and StoreBuilder.
        Hide
        Jacques Nadeau added a comment -

        resolved in a73512d

        Show
        Jacques Nadeau added a comment - resolved in a73512d

          People

          • Assignee:
            Ben Becker
            Reporter:
            Jacques Nadeau
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development