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

Enable the IN clause to also compare the value of single-pair structs #614

Closed
wants to merge 8 commits into from

Conversation

johnedquinn
Copy link
Member

@johnedquinn johnedquinn commented May 27, 2022

Related Issue

Related to #524

Description

Allows the IN clause to compare the value of single-pair structs, to maintain compatibility with SQL-92. To do this, it is necessary to make two changes:

  • If the rightOp is already computed as a sequence of single-pair structs (where each pair's value is a literal), we can use the optimized approach and immediately compare with that value.
  • If the rightOp is not computed (for example: another SELECT returning a bag/list of structs, or a bag of structs with pairs that contain un-evaluated values), then we need to evaluate.

Examples

SELECT age FROM << { 'name': 'John', 'age': 22 } >> WHERE name IN (SELECT name FROM <<{ 'name': 'John' }>>)

should result in:

<<
  { age: 22 }
>>

Notice

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dlurton
Copy link
Member

dlurton commented May 27, 2022

This change only applies to EvaluatingCompiler. Since we're intending to remove the EvaluatingCompiler at some point and its replacement isn't complete yet, there are two options:

  1. Make this change in EvaluatingCompiler and its replacement (PhysicalBexprToThunkConverter, introduced in Add Plan Evaluator #592 and will be merged to main today if all goes well).
  2. Make this change in only PhysicalBexprToThunkConverter, which means this fix will have limited applicability until the PhysicalBexprToThunkConverter is complete and EvaluatingCompiler is removed.

@johnedquinn
Copy link
Member Author

johnedquinn commented May 27, 2022

This change only applies to EvaluatingCompiler. Since we're intending to remove the EvaluatingCompiler at some point and its replacement isn't complete yet, there are two options:

  1. Make this change in EvaluatingCompiler and its replacement (PhysicalBexprToThunkConverter, introduced in Add Plan Evaluator #592 and will be merged to main today if all goes well).
  2. Make this change in only PhysicalBexprToThunkConverter, which means this fix will have limited applicability until the PhysicalBexprToThunkConverter is complete and EvaluatingCompiler is removed.

@dlurton -- Since this is working on a feature branch off of main, do you think it would make sense to create a separate PR to add to PhysicalBexprToThunkConverter? Then, this one can be merged immediately on approval, and I can work on merging to the other branch? Unless, of course, the other branch is expected to merge by EOD. Then, I'll just park this, pull once other is merged, then add to PhysicalBexprToThunkConverter and merge in the same PR later. Whatever works.

@dlurton
Copy link
Member

dlurton commented May 27, 2022

@johnedquinn I am hoping it can be merged by EOD today but that will depend on getting the approval of @alancai98

I would not make that separate PR due to the possibility of it being "inadvertently deprioritized".

Annnnnnnnd... it looks like he just approved it. I'll merge shortly...

@johnedquinn
Copy link
Member Author

@dlurton -- ACK. I'll park this and wait for the merge to main.

Action items:

RCHowell and others added 2 commits May 27, 2022 14:53
Merges the following PRs to `main`:

- #583 
- #584 
- #587 
- #588 
- #589 
- #609 
- #590 
- #592 

Also, this adds a TODO comment (ea40e4a, requested by @am357 in offline chat) and excludes `PlannerPipeline` from the new aggregate tests which were pulled in when merging from `main` and were not part of the other PRs (34025b3).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@johnedquinn johnedquinn marked this pull request as draft May 31, 2022 18:09
@johnedquinn
Copy link
Member Author

Accidentally merged all of the new changes in main into this feature branch. Starting a new one to make it easier for reviewers. Following up with PR #621 .

@johnedquinn johnedquinn deleted the fix-in-comparison branch December 5, 2022 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants