[
https://issues.apache.org/jira/browse/CALCITE-6574?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17881113#comment-17881113
]
James Starr edited comment on CALCITE-6574 at 9/11/24 8:02 PM:
---------------------------------------------------------------
I have renamed SqlQueryScopes to ScopeMap as requested. I added methods
getScopeMap to AbstractNamespace and ListScope to reduce the number of times
`validator.getScopeMap` form 40 to 18.
I mentioned the cyclic dependency in my initial email: namespaces require the
SqlValidatorImpl. Currently, AbstractNamespace, ListScope, and EmptyScope
depend on SqlValidatorImpl. Because of this, you cannot populate the scopes and
namespaces before constructing the SqlValidatorImpl. Decoupling this would
involve a lot of work and is beyond the scope of what I discussed. Furthermore,
requiring the SqlValidatorImpl to populate the scopes/namespaces is
counterproductive to the goal of populating the ScopeMap before constructing
the SqlValidatorImpl.
To rework the scope and namespace APIs to not depend on SqlValidatorImpl, the
logic for resolving types and validation would likely need to be moved out of
them and into a validation or scope/namespace step. The responsibility of the
namespace and scope interfaces spans multiple phases of converting a SQL node
to a rel node, thus cause these overly expansive dependencies.
I agree that adding getXXXScope introduces some verbosity on the calling side,
but splatting out a large interface only to immediately delegate to another
object is also verbose. If splatting out large interfaces is such a great
advantage, then why don’t we splat out the catalog into the validator?
The scopes and namespace currently use the validator api for:
* Resolving scopes and namespaces
* Creating Exceptions
* Type Lookup
* Inverting back to the validator.validate*
* Accessing the catalog reader, TypeFactory, Validator Config
* Propagating the SqlValidatorImpl to other things
* Exposing helper functions, such as isAggregate
* And more that I likely missed
Logically, the pipeline looks like:
```
String sqlString
SqlNode sqlNode = parse(sqlString)
ScopeMap scopeMap = populateScopes(sqlNodes);
TypeMap typeMap = validateTypes(sqlNodes, scopesMap);
RelNode relNode = convert(sqlNodes, scopeMap, typeMap);
```
Currently, populating the scope map and type map are tightly coupled. Their is
also an implicit context containing: type factory, exception factories, the
catalog, etc that is being glossed over with this.
[~julianhyde] Please let me know if this will work or if you want other
changes, otherwise I will abandon this effort as it is out of scope of what I
initially talked about.
was (Author: jamesstarr):
I have renamed SqlQueryScopes to ScopeMap as requested. I added methods
getScopeMap to AbstractNamespace and ListScope to reduce the number of times
`validator.getScopeMap` form 40 to 18.
I brought this cyclic dependency in my initial email that namespaces required
the SqlValidatorImpl. Currently, AbstractNamespace, ListScope, and EmptyScope
depend on SqlValidatorImpl. You can not populate the scopes and namespace
before constructing the SqlValidatorImpl because of this. Decoupling this
would be allot of work and well beyond the scope I have discussed.
Furthermore, requiring the SqlValidatorImpl to populate the scopes/namespace is
counter productive to the goal of populating the ScopeMap before the
construction of the SqlValidatorImpl. To rework the scope and namespace api to
not depend on the SqlValidatorImpl, the logic for resolving types, and
validating would likely need to be shifted out of them into a validation step
or scope/namespace step. The namespace and scope interfaces responsibility
span multiple phases of converting a sql node to a rel node.
I agree that adding getXXXScope adds a small bit of verbosity on the calling
side, but splatting out a large interface to just immediately delegate it to
another objects is also verbose. If splatting out large interfaces is such a
great win, then why don't we splat out the catalog into the validator?
The scopes and namespace currently use the validator api for:
* Resolving scopes and namespaces
* Creating Exceptions
* Type Lookup
* Inverting back to the validator.validate*
* Accessing the catalog reader, TypeFactory, Validator Config
* Propagating the SqlValidatorImpl to other things
* Exposing helper functions, such as isAggregate
* And more that I likely missed
Logically, the pipeline looks like:
```
String sqlString
SqlNode sqlNode = parse(sqlString)
ScopeMap scopeMap = populateScopes(sqlNodes);
TypeMap typeMap = validateTypes(sqlNodes, scopesMap);
RelNode relNode = convert(sqlNodes, scopeMap, typeMap);
```
Currently, populating the scope map and type map are tightly coupled. Their is
also an implicit context containing: type factory, exception factories, the
catalog, etc that is being glossed over with this.
[~julianhyde] Please let me know if this will work or if you want other
changes, otherwise I will abandon this effort as it is out of scope of what I
initially talked about.
> Add SqlQueryScopes
> ------------------
>
> Key: CALCITE-6574
> URL: https://issues.apache.org/jira/browse/CALCITE-6574
> Project: Calcite
> Issue Type: Sub-task
> Reporter: James Starr
> Assignee: James Starr
> Priority: Major
> Labels: pull-request-available
>
> See parent description.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
