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