I think at this point `BeamQueryPlanner` needs refactor. Basically it does 
multiple things and now even more than it did, plus violates encapsulation:
* creates a config;
* creates an instance of a planner using the config;
* exposes a bunch of implementation details on how it constructs the planner;
* exposes an interface to parse the query and convert it to RelNodes;

To me these look like this can be split into a bunch of independent small 
modules with fewer responsibilities and some inversion of control (each is 
simpler, easier to read and test):

```
class BeamCalciteFrameworkConfig {
   static FrameworkConfig create(CalciteConnection connection) { ... }
}

class CalcitePlannerFactory {
   Planner createCalcitePlanner(
                          FrameworkConfig calciteFrameworkConfig,
                          BeamSqlPipelineOptions pipelineOptions) {

      return  hasPlannerOption(pipelineOptions))
           ? createCustomPlanner(calciteFrameworkConfig)
           : Frameworks.getPlanner(calciteFrameworkConfig);
      }
   }
}
```

Rename BeamQueryPlanner to something like `QueryParser`

```
class QueryParser {
   static create(Planner calcitePlanner) {
      this.planner = calcitePlanner;
   }
}
```

Then just configure everything within `BeamSqlEnv`:

```
class BeamSqlEnv {
   private BeamSqlEnv(...) {
      ...
      calciteFrameworkConfig = BeamCalciteFrameworkConfig.create(connection);
      calcitePlannerImpl = CalcitePlannerFactory.create(calciteFrameworkConfig, 
getPipelineOptions());
      queryParser = QueryParser.create(calcitePlannerImpl);
   }
}
```

[ Full content available at: https://github.com/apache/beam/pull/6598 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org

Reply via email to