Conversation
Adding SqlQueryScopes to encapsulate the data for resolving SqlNode to SqlValidatorScope and SqlValidatorNamespaces. Moving the api for resolving scopes from SqlValidator to SqlQueryScopes.
… using a generic Fuction in SqlValiator.Config.
AbstractNamespace and ListScope.
Adding NamespaceBuilder to formalize the api for customizing/reusing namespaces downstream.
|
Looks pretty good. I'm tempted to make the following next steps:
|
…is allows for overriding validateFeature. Removing unused getter from ValidatorAggStuff.
jamesstarr
force-pushed
the
wip_scope_populator
branch
from
65de4db to
1139aba
Compare
@julianhyde, the ScopeMap interface is already read-only, which provides most of the clarity we gain from immutability. The additional clarity from introducing an explicit immutable version of ScopeMap is marginal, while the complexity introduced is significant. This is due to your desire for a redelegation layer in SqlValidator, the existing circular dependencies between ScopeMap, namespaces, and SqlValidator, and the non-trivial methods in ScopeMapImpl, which need to support downstream overriding while not have code duplication. In addition, the scopeMap reference would have to mutate within SqlValidatorImpl, which would create a complex lifecycle and essentially negate the intended clarity. Allowing downstream projects to override ScopeMapImpl is why it's currently package-scoped. Moreover, several methods in ScopeMap and ScopeMapImpl are public or protected, so we'd need to provide an API for overriding those methods moving forward, unless we plan to offer an upgrade path for projects that have customized them. Given the complexity of these interactions and the time it would take to implement, I’m not willing to introduce an immutable version of ScopeMap at this time. However, the work on the ScopeMap interface will make it easier to add an immutable version later, once the circular dependencies are resolved. I can add a redegalation layer to SqlValidator, but please be very explicit on why you want this, because I do not understand. All it seems to do is adding 200 lines of boilerplate while making api opaque and creating future work. |
jamesstarr
force-pushed
the
wip_scope_populator
branch
from
d810dcc to
155ab8f
Compare
|
|
@julianhyde Any thoughts on wether this is worth going forward? |
|
@julianhyde Would it be ok if asked @mihaibudiu to review this? |
|
I can try to review it, but @julianhyde knows at least 10^6 times more about this software than I do |
|
@mihaibudiu Lets wait for @julianhyde blessing to go forward before. I do not want to waste your time. |
|
Reading code is a great way of learning about how the system works, and since I only have a vague understanding of the validator, it could be useful to me. The main problem is that I couldn't come up with most arguments that @julianhyde made, because I don't understand many of the reasons behind the current design and their implications. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions. |
3 participants
No description provided.