Closes #634 - Fix variables in predicates #635
Conversation
| .and_then(|cols| cols.first().map(|col| QueryValue::Column(col.clone()))) | ||
| .ok_or_else(|| Error::from_kind(ErrorKind::UnboundVariable(var.name()))) | ||
| match self.bound_value(&var) { | ||
| // The type is already known if it's a bound variable…. |
There was a problem hiding this comment.
This comment doesn't make sense for this particular function.
| match self.bound_value(&var) { | ||
| // The type is already known if it's a bound variable…. | ||
| Some(TypedValue::Long(v)) => Ok(QueryValue::TypedValue(TypedValue::Long(v))), | ||
| Some(TypedValue::Double(v)) => Ok(QueryValue::TypedValue(TypedValue::Double(v))), |
There was a problem hiding this comment.
This isn't quite right. If it's already bound to the wrong kind of value, we'll still fall through to the column binding and get an UnboundVariable error.
I think you want something like:
Some(v) => {
bail!(ErrorKind::InputTypeDisagreement(var.name.clone(), ValueType::Long, v.value_type()));
},
and similar on line 85.
There was a problem hiding this comment.
I also think you want to add ValueType::is_numeric to core, and here do:
Some(v) if v.value_type().is_numeric() => Ok(QueryValue::TypedValue(v)),
Some(v) => { bail!(…); },
— we will at some point add BigInteger support, which would introduce a bug here.
There was a problem hiding this comment.
Hmm, I don't think I can do this. The guard statement claims ownership so it's not available to the case body. If I make v a reference, the guard works but I cannot claim ownership to return the value inside the body without cloning it.
Also looks like there is ongoing discussion about this rust-lang/rfcs#2036
I will try and do something else but similar. This is where I miss Swifts chaning if-lets and guards so I could do:
guard let v = self.bound_value(&var),
v.value_type().is_numeric() else { ... }
fluffyemily
force-pushed
the
fluffyemily/issue-634-predicate-variables
branch
from
78bb9bf to
7c9bfc4
Compare
| .ok_or_else(|| Error::from_kind(ErrorKind::UnboundVariable(var.name()))) | ||
| match self.bound_value(&var) { | ||
| Some(TypedValue::Instant(v)) => Ok(QueryValue::TypedValue(TypedValue::Instant(v))), | ||
| _ => { |
There was a problem hiding this comment.
Same review comment as before:
This isn't quite right. If it's already bound to the wrong kind of value, we'll still fall through to the column binding and get an
UnboundVariableerror.
You need a similar bail line here as there is on line 53.
|
@fluffyemily don't forget to wrap this up… |
fluffyemily
force-pushed
the
fluffyemily/issue-634-predicate-variables
branch
from
7c9bfc4 to
d67cb74
Compare
|
Finally. Not sure why I forgot about this. |
No description provided.