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

Support contains, exists, forall for optional embedded case classes with optional fields. #838

Merged
merged 1 commit into from
Jul 18, 2017

Conversation

mxl
Copy link
Contributor

@mxl mxl commented Jul 16, 2017

Fixes #785

Solution

  • flatten option operations recursively to support optional embedded case classes with optional fields
  • parse embedded.forall(...) as OptionExists AST

Notes

embedded.forall(...) currently generates invalid query like (t.embedded IS NULL) || ....
One option is to generate instead - ((t.a IS NULL) && (t.b IS NULL)) || ... where a, b is embedded fields, but it's too hard because on FlattenOptionOperation stage there is no info about embedded case classes and their fields. Also this behavior will be too complex and unexpected (especially if there will be nested embedded case classes). Without this behavior embedded.forall(...) is equivalent to embedded.exists(...). I thought about adding warning on parsing stage to motivate developers use exists() instead of forall() with embedded case classes but I do not think that someone will expect from forall() the behavior that I described above (with generating ((t.a IS NULL) && (t.b IS NULL)) || ...).

Checklist

  • Unit test all changes
  • Update README.md if applicable
  • Add [WIP] to the pull request title if it's work in progress
  • Squash commits that aren't meaningful changes
  • Run sbt scalariformFormat test:scalariformFormat to make sure that the source files are formatted

@getquill/maintainers

@mxl mxl added the ready label Jul 16, 2017
@mxl mxl force-pushed the fix-785 branch 2 times, most recently from 6cc2069 to 7c51496 Compare July 17, 2017 12:58
@fwbrasil
Copy link
Collaborator

I think it'd be better to fail if forall is used with an embedded value. The parsed AST should mirror the original code.

Flatten option operations recursively to make them work with optional embedded case classes with optional fields.
Parse forall on Embedded as OptionExists AST because embedded.forall(_ == None) makes no sense.
@mxl
Copy link
Contributor Author

mxl commented Jul 18, 2017

@fwbrasil I fixed that 👍

Copy link
Collaborator

@fwbrasil fwbrasil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! :)

@mxl mxl merged commit afffedc into zio:master Jul 18, 2017
@mxl mxl deleted the fix-785 branch July 18, 2017 21:29
@mxl mxl restored the fix-785 branch October 29, 2017 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants