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

qb feature request: Literal value specification in "set", "where" and "if" #19

Closed
jgiles opened this issue Oct 3, 2017 · 10 comments
Closed

Comments

@jgiles
Copy link
Contributor

jgiles commented Oct 3, 2017

In cases where queries are setting or comparing against some constant value (e.g. NULL, 0, "", a particular value of an enum-style text field), it would improve usability and readability to specify that constant value as a literal during query construction rather than making it a query parameter and then binding it later.

A natural API might be to add "Lit" variants in the same way there are currently "Named" variants:
qb.EqLit(column string, value string)
qb.GtLit(column string, value string)
qb.InLit(column string, valueSet string...)
...
and probably a .SetLit(column string, value string)

I've made the inputs strings here to avoid the question of value marshaling - if the input really is constant or constant-like, writing out the string should not be too burdensome for clients.

Or, if that adds unacceptable bloat, there could be some
qb.CmpLit(expression string)
where the caller is responsible for including the operator. That reduces API size and may be the most flexible, but it opens a fairly inviting hole for misuse in an otherwise pretty prescriptive API.

I'm happy to send a PR implementing this (and almost just sent one), but figured it would be good to discuss the desired API first.

@mmatczuk
Copy link
Contributor

mmatczuk commented Oct 3, 2017

Hi @jgiles,
thanks for the proposal! I like the "Lit" idea because it improves readability so we can do it.

@jgiles
Copy link
Contributor Author

jgiles commented Oct 3, 2017

So, a 'Lit' suffix on on the Eq, Gt, etc?

@mmatczuk
Copy link
Contributor

mmatczuk commented Oct 3, 2017

Yes.

@jgiles
Copy link
Contributor Author

jgiles commented Oct 3, 2017

Sweet, I'll send a PR (possibly in a couple days, I have a few other things in the queue at the moment).
Thanks!

@mmatczuk
Copy link
Contributor

mmatczuk commented Oct 3, 2017

Sure, no worries. Thanks!

@mmatczuk
Copy link
Contributor

Hi @jgiles how are you doing? I'm thinking of taking over this task, I do not want to get in your way.

@jgiles
Copy link
Contributor Author

jgiles commented Oct 29, 2017

@mmatczuk "in a couple days" - famous last words. I should have a PR up today, apologies for the delayed response.

jgiles added a commit to paxosglobal/DEPRECATED-gocqlx that referenced this issue Oct 29, 2017
This adds *Lit variants of the Eq, Gt, Lt, etc comparison methods,
permitting convenient comparison against known constants.

Restructure Cmp.writeCql to avoid appends + named-parameter return for
clarity, which also reduces BenchmarkCmp-8 from 612 ns/op to 555 ns/op.
jgiles added a commit to paxosglobal/DEPRECATED-gocqlx that referenced this issue Oct 29, 2017
This adds *Lit variants of the Eq, Gt, Lt, etc comparison methods,
permitting convenient comparison against known constants.

Restructure Cmp.writeCql to avoid appends + named-parameter return for
clarity, which also reduces BenchmarkCmp-8 from 612 ns/op to 555 ns/op.
@jgiles
Copy link
Contributor Author

jgiles commented Oct 29, 2017

In fact, I went just went ahead and put up #22 which does the comparison changes.

jgiles added a commit to paxosglobal/DEPRECATED-gocqlx that referenced this issue Oct 29, 2017
This fixes scylladb#19 and expands on its ideas a little bit.

The 'value' interface represents a CQL value for use in a comparison,
update, or intitialization operation. A consistent interface for this
allows us to easily support specifying default-named, custom-named,
literal, and evaluated-function values in all these contexts.

Parameters to Func should probably also be values to support full
composition, but that would be a breaking change because Func's
properties are exposed.

The value interface could itself be exposed if we wanted to allow
clients to pass their own values to SetValue, etc, but for now it is a
package-internal abstraction.

The 'clone' method is a precursor to scylladb#23, but for now serves to prevent
any type with the writeCql method from being accepted as a value.

The benchmarks don't change much, except for insert which gets slower
because of the extra work we are doing. But, I don't think that cost
matters given the feature improvement.
jgiles added a commit to paxosglobal/DEPRECATED-gocqlx that referenced this issue Oct 29, 2017
This fixes scylladb#19 and expands on its ideas a little bit.

The 'value' interface represents a CQL value for use in a comparison,
update, or intitialization operation. A consistent interface for this
allows us to easily support specifying default-named, custom-named,
literal, and evaluated-function values in all these contexts.

Parameters to Func should probably also be values to support full
composition, but that would be a breaking change because Func's
properties are exposed.

The value interface could itself be exposed if we wanted to allow
clients to pass their own values to SetValue, etc, but for now it is a
package-internal abstraction.

The 'clone' method is a precursor to scylladb#23, but for now serves to prevent
any type with the writeCql method from being accepted as a value.

The benchmarks don't change much, except for insert which gets slower
because of the extra work we are doing. But, I don't think that cost
matters given the feature improvement.
@jgiles
Copy link
Contributor Author

jgiles commented Oct 29, 2017

#22 is now a more comprehensive fix, let me know what you think @mmatczuk

@mmatczuk
Copy link
Contributor

Closed in #37.

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

No branches or pull requests

2 participants