Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Variable resolution and dynamic lookup evaluation incorrect or performing too many lookups #410

Closed
alancai98 opened this issue Jul 17, 2023 · 0 comments · Fixed by #416
Closed
Labels
bug Something isn't working

Comments

@alancai98
Copy link
Member

Variable resolution currently checks the local bindings and then searches the global context for each lookup:

let value = Bindings::get(bindings, &self.name).or_else(|| ctx.bindings().get(&self.name));

There is currently no way to perform the global lookup first and then the local binding lookup that is required when a variable or path in the FROM source is unqualified as specified in section 10 of the spec. This is apparent in a test case such as:

-- assuming a global table is defined as `foo`
SELECT a FROM foo, foo AS t2

The LHS of the cross join will be correct in pulling foo from the globals since the local bindings are empty. This will put foo in the local bindings. However the RHS of the cross join should check globals first (from section 10 of the spec) but will check the locals bindings first due to the current variable reference implementation.


Another consequence of the current modeling of variable references is that dynamic lookup will check the global context for every local variable lookup. For instance if we have some dynamic lookup of [path(local(a)), path(local(b)), path(local(c)), varref(global(d))], the actual lookup will do something like:

  1. local lookup of a; if nothing, then global lookup of a
  2. if nothing, local lookup of b; if nothing, then global lookup of b
  3. if nothing, local lookup of c; if nothing, then global lookup of c
  4. if nothing, global lookup of d
  5. else Missing

when it should be something like:

  1. local lookup of a
  2. if nothing, local lookup of b
  3. if nothing, local lookup of c
  4. if nothing, global lookup of d
  5. else Missing
@alancai98 alancai98 added the bug Something isn't working label Jul 17, 2023
@alancai98 alancai98 linked a pull request Jul 27, 2023 that will close this issue
am357 added a commit that referenced this issue Aug 16, 2023
During the work for adding a steel thread for PartiQL Typing (Recent PR #389)
and after merging #410 to `feat-type-plan-poc` it is realized that we need to
refactor the code to remove `DynamicLocalup` `VarExpr` with the assumption that
we work based off of Typing and Value Environment from the Catalog. We have a
Typing Environment in the Catalog at the moment and we are going to add the
Variable Environment as well. In preparation for such task, we need to make
the `NameResolver` Catalog aware. In that regard this commit adds the `Catalog`
to `NameResolver`

Expecting subsequent PR(s) for the name resolving using the Catalog.
am357 added a commit that referenced this issue Aug 16, 2023
During the work for adding a steel thread for PartiQL Typing (Recent PR #389)
and after merging #410 to `feat-type-plan-poc` it is realized that we need to
refactor the code to remove `DynamicLocalup` `VarExpr` with the assumption that
we work based off of Typing and Value Environment from the Catalog. We have a
Typing Environment in the Catalog at the moment and we are going to add the
Variable Environment as well. In preparation for such task, we need to make
the `NameResolver` Catalog aware. In that regard this commit adds the `Catalog`
to `NameResolver`

Expecting subsequent PR(s) for the name resolving using the Catalog.
am357 added a commit that referenced this issue Aug 16, 2023
During the work for adding a steel thread for PartiQL Typing (Recent PR #389)
and after merging #410 to `feat-type-plan-poc` it is realized that we need to
refactor the code to remove `DynamicLocalup` `VarExpr` with the assumption that
we work based off of Typing and Value Environment from the Catalog. We have a
Typing Environment in the Catalog at the moment and we are going to add the
Variable Environment as well. In preparation for such task, we need to make
the `NameResolver` Catalog aware. In that regard this commit adds the `Catalog`
to `NameResolver`

Expecting subsequent PR(s) for the name resolving using the Catalog.
am357 added a commit that referenced this issue Aug 16, 2023
During the work for adding a steel thread for PartiQL Typing (Recent PR #389)
and after merging #410 to `feat-type-plan-poc` it is realized that we need to
refactor the code to remove `DynamicLocalup` `VarExpr` with the assumption that
we work based off of Typing and Value Environment from the Catalog. We have a
Typing Environment in the Catalog at the moment and we are going to add the
Variable Environment as well. In preparation for such task, we need to make
the `NameResolver` Catalog aware. In that regard this commit adds the `Catalog`
to `NameResolver`

Expecting subsequent PR(s) for the name resolving using the Catalog.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant