[
https://issues.apache.org/jira/browse/CASSANDRA-11935?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15565286#comment-15565286
]
Sylvain Lebresne commented on CASSANDRA-11935:
----------------------------------------------
General functionality lgtm. A bunch of remark though:
* This is more for the record (not really asking anything here), but the
grammar is starting to be unreadable imo. I don't meant that as a criticism of
this patch, as I don't have a better idea for dealing with this without a much
larger refactor of the CQL code, but we may want to keep an eye on this moving
forward. This also mean that while I've looked at the grammar changes and did
not notice an obvious problem, I want to be clear that I'm not confident in my
own capacity to vet grammar changes properly at this point.
* It's not fully clear to me what purposes the {{WithOperation}} and
{{WithNegation}} classes in {{Selectable}} serves. I would *really* prefer we
deal with operators as normal functions internally as much as possible
(operators are "just" syntactic sugar for true functions calls) and so I'd
rather use {{WithFunction}} directly in both cases.
* I'm also unsure about {{Selectable.BetweenParentheses}}. It seems to assume
we can't have a tuple of size 1 but I don't think that's true. I believe we
should do as for {{WithMapOrUDT}}: keep it a tuple/undetermined until we get
the type and decide then. Though we probably need to be careful with things
like {{((1))}} (where the internal one is a tuple) (meaning, we should handle
it and have a test for it).
* The patch adds extended support for collections in select statement. Namely,
you can now do {{SELECT \[a, f(a)\] FROM...}} which didn't worked before. It's
nice (though unrelated to this ticket and ideally would have been kept for a
follow up) but I don't see much test of this.
* Not sure about the hard-coding of {{MathContext.DECIMAL128}} as
precision/rounding settings for {{BigDecimal}}. As in, I'm not knowledgable
enough on decimal arithmetic to decide if this is the best choice or not. But
in doubt, I would have maybe stick to the default {{BigDecimal}} behavior.
* On {{Type.getPreferedTypeFor()}}:
** the javadoc is kind of wrong: it wouldn't be called "getPreferedType" if it
was returning "the exact type". I think it'll also be useful to call out when
that function is used (namely, during during function resolution to force a
choice between multiple possible overrides).
** not sure why we stop at {{int}} for integers. Why not return a {{tinyint}}
if it fits? I'd expect users to be surprised if {{c + 1}} returns a {{int}}
when {{c}} is a {{tinyint}}/{{smallint}}, especially when {{c + c}} does return
a {{tinyint}}/{{smallint}}. Worst, with {{c}} still a {{tinyint}}, this means
{{WHERE c = 2}} works but {{WHERE c = 1 + 1}} doesn't.
** It's a nit I suppose, but smallish values are likely much more common, so
creating {{BigInteger}}//{{BigDecimal}} by default feels wasteful. I would have
done a {{Long.parseLong()}} by default and fallback on {{BigInteger}} on
failure.
** Maybe it's worth going though the end of the idea and implement that method
for some other literals? At least UUIDs could prefer {{TimeUUIDType}} if it
looks like a version 1 uuid, {{UUIDType}} otherwise (if you have 2 overrides,
one for {{TimeUUIDType}} and {{UUIDType}}, what are the changes you don't want
to pick the {{TimeUUIDType}} one if the string is a v1 uuid? And it's actually
somewhat stupid being ambiguous if the uuid isn't a v1 uuid). No reason to no
return something for {{HEX}} and {{BOOLEAN}} either, even though this actually
won't have any practical consequence (outside of making the code look less
arbitrary that is). Strings are little more debatable though, so happy to leave
them out for now.
* In {{OperationFcts}}, making an enum for the operations:
{code}
public enum OPERATOR
{
ADDITION("+", "_add"),
SUBSTRACTION("-", "_substract"),
...
}
{code}
would allow making some of the code a tiny bit cleaner imo. In particular,
instead of having {{add}}, {{substract}}, ... methods in {{NumberType}}, we
could just have a single {{compute}} that takes the operation, which would
reduce the amount of boilerplate in the implementations of those method (for a
given type, all have the exact same body except for the operation). Can also
avoid the 5 boilerplate classes in {{OperationFcts}}.
* Regarding the {{NumberType}} new methods: the code does tons of
boxing/unboxing for not that much clarity benefit imo (granted, we have another
inefficiency in the fact that we serialize/deserialize to bytes for every
function call, and we should certainly fix that at some point. No reason to be
more inefficient that we have to in the meantime though). Anyway, I'd probably
put all the {{toXXX()}} method in {{NumberType}} as "unsupported" and override
with a non-boxing implementation when appropriate.
* The last one is more of a nitpick because it doesn't matter much in practice,
but the {{OperationFcts.returnType()}} function can do quite a bit of
comparisons and I don't find it easy to proof-read: the logic, which is to pick
the precision of the bigger argument and favor floating point if any operand
is, isn't easily apparent. So I'd have gone with something along the lines of:
{code}
private static NumberType<?> returnType(NumberType<?> left, NumberType<?> right)
{
boolean isFloatingPoint = isFloatingPoint(left) || isFloatingPoint(right);
int precision = Math.max(precision(left), precision(right));
return isFloatingPoint
? floatPointType(precision)
: integerType(precision);
}
private static boolean isFloatingPoint(NumberType<?> type)
{
switch ((Native)type.asCQL3Type())
{
case DECIMAL:
case DOUBLE:
case FLOAT:
return true;
default:
return false;
}
}
private static int precision(NumberType<?> type)
{
switch ((Native)type.asCQL3Type())
{
case DECIMAL:
case VARINT:
return Integer.MAX_VALUE;
case DOUBLE:
case BIGINT:
case COUNTER:
return 8;
case INT:
case FLOAT:
return 4;
case SMALLINT;
return 2;
case TINYINT;
return 1;
}
}
private static NumberType<?> floatPointType(int precision)
{
switch (precision)
{
case 4: return FloatType.instance;
case 8: return DoubleType.instance;
case Integer.MAX_VALUE: return DecimalType.instance;
}
}
private static NumberType<?> integerType(int precision)
{
switch (precision)
{
case 1: return ByteType.instance;
case 2: return ShortType.instance;
case 4: return Int32Type.instance;
case 8: return LongType.instance;
case Integer.MAX_VALUE: return IntegerType.instance;
}
}
{code}
> Add support for arithmetic operators
> ------------------------------------
>
> Key: CASSANDRA-11935
> URL: https://issues.apache.org/jira/browse/CASSANDRA-11935
> Project: Cassandra
> Issue Type: Sub-task
> Components: CQL
> Reporter: Benjamin Lerer
> Assignee: Benjamin Lerer
> Fix For: 3.x
>
>
> The goal of this ticket is to add support for arithmetic operators:
> * {{-}}: Change the sign of the argument
> * {{+}}: Addition operator
> * {{-}}: Minus operator
> * {{*}}: Multiplication operator
> * {{/}}: Division operator
> * {{%}}: Modulo operator
> This ticket we should focus on adding operator only for numeric types to keep
> the scope as small as possible. Dates and string operations will be adressed
> in follow up tickets.
> The operation precedence should be:
> # {{*}}, {{/}}, {{%}}
> # {{+}}, {{-}}
> Some implicit data conversion should be performed when operations are
> performed on different types (e.g. double + int).
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
