cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sylvain Lebresne (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-11935) Add support for arithmetic operators
Date Tue, 11 Oct 2016 12:27:21 GMT

    [ 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)

Mime
View raw message