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.

Sign up for GitHub

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

Jump to bottom

Add support of 'contains' operation on Cassandra collections #813

Merged
merged 1 commit into from
Sep 11, 2017
Merged

Add support of 'contains' operation on Cassandra collections #813

merged 1 commit into from
Sep 11, 2017

Conversation

mosyp
Copy link
Collaborator

@mosyp mosyp commented Jun 30, 2017 •
edited
Loading

Fixes partially #774

Problem

Quill does not provide support for contains on collections

Solution

Provide support for contains on collections

Notes

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

@mosyp mosyp changed the title Add support of contains on Cassandra collections Add support of 'contains' operation on Cassandra collections Jun 30, 2017
@mosyp mosyp changed the title Add support of 'contains' operation on Cassandra collections [WIP]Add support of 'contains' operation on Cassandra collections Jun 30, 2017
@mosyp mosyp force-pushed the cassandra-collections-operations branch from bd3b9c8 to feed4be Compare July 1, 2017 07:54
@mosyp mosyp force-pushed the cassandra-collections-operations branch 6 times, most recently from 0730fc3 to ee21096 Compare September 10, 2017 21:21
@mosyp mosyp changed the title [WIP]Add support of 'contains' operation on Cassandra collections Add support of 'contains' operation on Cassandra collections Sep 10, 2017
@mosyp
Copy link
Collaborator Author

mosyp commented Sep 10, 2017 •
edited
Loading

contains on collection requires ALLOW FILTERING however I don't see the need to complicate the parsing/verification, hence note in documentation is enough. Otherwise exception is thrown: Cause: com.datastax.driver.core.exceptions.InvalidQueryException: Cannot execute this query as it might involve data filtering and thus may have unpredictable performance. If you want to execute this query despite the performance unpredictability, use ALLOW FILTERING

fwbrasil
fwbrasil reviewed Sep 11, 2017
View reviewed changes
quill-core/src/main/scala/io/getquill/quotation/Parsing.scala Outdated
@@ -49,6 +49,7 @@ trait Parsing {
case `propertyParser`(value) => value
case `stringInterpolationParser`(value) => value
case `optionOperationParser`(value) => value
case `traversableOperationParser`(value) => value
Copy link
Collaborator

Choose a reason for hiding this comment

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

formatting seems off

fwbrasil
fwbrasil reviewed Sep 11, 2017
View reviewed changes
quill-orientdb/src/main/scala/io/getquill/context/orientdb/OrientDBIdiom.scala Outdated
@@ -60,7 +60,7 @@ trait OrientDBIdiom extends Idiom {
a.token
case a @ (
_: Function | _: FunctionApply | _: Dynamic | _: OptionOperation | _: Block |
_: Val | _: Ordering | _: QuotedReference
_: Val | _: Ordering | _: QuotedReference | _: TraversableOperation
) =>
fail(s"Malformed query $a.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you change the error to something like Malformed or unsupported construct: $a

fwbrasil
fwbrasil reviewed Sep 11, 2017
View reviewed changes
quill-sql/src/main/scala/io/getquill/context/sql/idiom/SqlIdiom.scala
@@ -60,7 +60,7 @@ trait SqlIdiom extends Idiom {
case a: Assignment => a.token
case a @ (
_: Function | _: FunctionApply | _: Dynamic | _: OptionOperation | _: Block |
_: Val | _: Ordering | _: QuotedReference
_: Val | _: Ordering | _: QuotedReference | _: TraversableOperation
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

fwbrasil
fwbrasil approved these changes Sep 11, 2017
View reviewed changes
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.

besides the minor issues, LGTM. Thanks @mentegy!

@fwbrasil
Copy link
Collaborator

the build seems to fail because tut detects errors in the readme

@mosyp mosyp force-pushed the cassandra-collections-operations branch from ee21096 to 4a1547f Compare September 11, 2017 06:29
@mosyp mosyp force-pushed the cassandra-collections-operations branch from 4a1547f to 271d751 Compare September 11, 2017 08:42
@mosyp
Copy link
Collaborator Author

mosyp commented Sep 11, 2017

@fwbrasil 👍

@mosyp mosyp merged commit 06e2efe into zio:master Sep 11, 2017
@mosyp mosyp deleted the cassandra-collections-operations branch September 11, 2017 09:21
@mosyp mosyp mentioned this pull request Dec 11, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fwbrasil fwbrasil fwbrasil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mosyp @fwbrasil @mentegy