[jira] [Commented] (CASSANDRA-15232) Arithmetic operators over decimal truncate results
[ https://issues.apache.org/jira/browse/CASSANDRA-15232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16910955#comment-16910955 ] Liudmila Kornilova commented on CASSANDRA-15232: :) > Arithmetic operators over decimal truncate results > -- > > Key: CASSANDRA-15232 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15232 > Project: Cassandra > Issue Type: Improvement > Components: CQL/Semantics >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 4.0 > > Time Spent: 1.5h > Remaining Estimate: 0h > > The decimal operators hard-code a 128 bit precision for their computations. > Probably a precision needs to be configured or decided somehow, but it’s not > clear why 128bit was chosen. Particularly for multiplication and addition, > it’s very unclear why we truncate, which is different to our behaviour for > e.g. sum() aggregates. Probably for division we should also ensure that we > do not reduce the precision of the two operands. A minimum of decimal128 > seems reasonable, but a maximum does not. -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15232) Arithmetic operators over decimal truncate results
[ https://issues.apache.org/jira/browse/CASSANDRA-15232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16903591#comment-16903591 ] Liudmila Kornilova commented on CASSANDRA-15232: 1. (y) 2. I inlined expressions and added a comment I'll create a ticket about modulo operation for floating point types I'll take a look at CASSANDRA-15269. Even if this commit is merged first, it will be possible to work on the problem by reverting this commit locally > Arithmetic operators over decimal truncate results > -- > > Key: CASSANDRA-15232 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15232 > Project: Cassandra > Issue Type: Improvement > Components: CQL/Semantics >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 4.0 > > Time Spent: 1.5h > Remaining Estimate: 0h > > The decimal operators hard-code a 128 bit precision for their computations. > Probably a precision needs to be configured or decided somehow, but it’s not > clear why 128bit was chosen. Particularly for multiplication and addition, > it’s very unclear why we truncate, which is different to our behaviour for > e.g. sum() aggregates. Probably for division we should also ensure that we > do not reduce the precision of the two operands. A minimum of decimal128 > seems reasonable, but a maximum does not. -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15232) Arithmetic operators over decimal truncate results
[
https://issues.apache.org/jira/browse/CASSANDRA-15232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16902898#comment-16902898
]
Benedict commented on CASSANDRA-15232:
--
Thanks [~Override], I agree that's the best way forward.
The patch is almost ready to commit. I have a couple of nits for you to
consider:
# {{CONTEXT_WITH_MAX_PRECISION}} and {{MAX_PRECISION}} could be merged into
simply {{MAX_PRECISION}} (as a {{MathContext}}) for brevity, since
{{MAX_PRECISION}} isn't used independently?
# {{leftOperandFirstDigitPos}} and {{rightOperandFirstDigitPos}} both subtract
1, which is cancelled out by the subtraction of one from the other, and are no
longer used for any other calculation. Perhaps rename the variables to
something that indicates we're just getting the normalised scale wrt each
other's decimal point? I haven't come up with an immediately good name, but
I'm sure it's possible. Of course, I hope the compiler eliminates the extra
calculation for us, so it's not strictly necessary - but it helps to avoid
triggering any human linters in future, and more clearly defines the purpose of
the variables.
Otherwise the patch looks ready to commit to me, thanks again for another great
submission.
Reviewing this ticket highlights another follow-up ticket, or discussion,
around the mod operator: I'm not entirely sure makes a lot of sense over
decimal values. Is this definitely a function we want to provide? I've only
ever understood it to be defined over the integer domain. I note that
{{BigDecimal}} names it {{remainder}}, and in fact the Java Language Spec does
the same, so it is not modulus we are offering here and we should rename it
internally and in any docs anyway.
We should probably discuss this for all value types, particularly floating
point types. I honestly was unaware Java supported remainder for floating
point, and the behaviour is unintuitive to say the least ({{0.4 % 0.25 ==
0.15}} ??)
I've also noted your other follow-up, it's good we're shaking out these minor
issues before we release the feature, so your help here is greatly appreciated!
> Arithmetic operators over decimal truncate results
> --
>
> Key: CASSANDRA-15232
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15232
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL/Semantics
>Reporter: Benedict
>Assignee: Liudmila Kornilova
>Priority: Normal
> Labels: pull-request-available
> Fix For: 4.0
>
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> The decimal operators hard-code a 128 bit precision for their computations.
> Probably a precision needs to be configured or decided somehow, but it’s not
> clear why 128bit was chosen. Particularly for multiplication and addition,
> it’s very unclear why we truncate, which is different to our behaviour for
> e.g. sum() aggregates. Probably for division we should also ensure that we
> do not reduce the precision of the two operands. A minimum of decimal128
> seems reasonable, but a maximum does not.
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15232) Arithmetic operators over decimal truncate results
[ https://issues.apache.org/jira/browse/CASSANDRA-15232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16902870#comment-16902870 ] Liudmila Kornilova commented on CASSANDRA-15232: Hello [~benedict], I removed calculation of first digits. In postgres it makes sense because the operation requires only array element accesses. Also it's a heuristic that does not always lead to better result (in case when first to digits are equal) > Arithmetic operators over decimal truncate results > -- > > Key: CASSANDRA-15232 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15232 > Project: Cassandra > Issue Type: Improvement > Components: CQL/Semantics >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 4.0 > > Time Spent: 1.5h > Remaining Estimate: 0h > > The decimal operators hard-code a 128 bit precision for their computations. > Probably a precision needs to be configured or decided somehow, but it’s not > clear why 128bit was chosen. Particularly for multiplication and addition, > it’s very unclear why we truncate, which is different to our behaviour for > e.g. sum() aggregates. Probably for division we should also ensure that we > do not reduce the precision of the two operands. A minimum of decimal128 > seems reasonable, but a maximum does not. -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15232) Arithmetic operators over decimal truncate results
[ https://issues.apache.org/jira/browse/CASSANDRA-15232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901242#comment-16901242 ] Liudmila Kornilova commented on CASSANDRA-15232: Hi, [~benedict] I'll go through the code one more time tomorrow and "Submit patch"! > Arithmetic operators over decimal truncate results > -- > > Key: CASSANDRA-15232 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15232 > Project: Cassandra > Issue Type: Improvement > Components: CQL/Semantics >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 4.0 > > Time Spent: 1h 20m > Remaining Estimate: 0h > > The decimal operators hard-code a 128 bit precision for their computations. > Probably a precision needs to be configured or decided somehow, but it’s not > clear why 128bit was chosen. Particularly for multiplication and addition, > it’s very unclear why we truncate, which is different to our behaviour for > e.g. sum() aggregates. Probably for division we should also ensure that we > do not reduce the precision of the two operands. A minimum of decimal128 > seems reasonable, but a maximum does not. -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15232) Arithmetic operators over decimal truncate results
[ https://issues.apache.org/jira/browse/CASSANDRA-15232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901240#comment-16901240 ] Benedict commented on CASSANDRA-15232: -- Oh, also, it helps if you "Submit Patch", so I can "Start Review" etc :) > Arithmetic operators over decimal truncate results > -- > > Key: CASSANDRA-15232 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15232 > Project: Cassandra > Issue Type: Improvement > Components: CQL/Semantics >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 4.0 > > Time Spent: 1h 20m > Remaining Estimate: 0h > > The decimal operators hard-code a 128 bit precision for their computations. > Probably a precision needs to be configured or decided somehow, but it’s not > clear why 128bit was chosen. Particularly for multiplication and addition, > it’s very unclear why we truncate, which is different to our behaviour for > e.g. sum() aggregates. Probably for division we should also ensure that we > do not reduce the precision of the two operands. A minimum of decimal128 > seems reasonable, but a maximum does not. -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15232) Arithmetic operators over decimal truncate results
[
https://issues.apache.org/jira/browse/CASSANDRA-15232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901239#comment-16901239
]
Benedict commented on CASSANDRA-15232:
--
Thanks for the insight [~blerer] :)
[~Override] thanks for the patch. It looks good. I have only one concern,
around division, namely how the scale calculation is now potentially costlier
than the division itself. This may not be a huge problem, but it is a shame,
and it would be nice to mitigate it.
The costliest part is calculating the first digit of each operand - I
personally think it would be acceptable to ignore this part of Postgres'
calculation to avoid introducing the extra garbage and computation.
However, if we do want to retain this part, we should at least avoid performing
it when we know it is of no value - i.e. when the scale of the result is such
that we must ignore the contribution of these digits, because we know for sure
the scale will be overridden by min or max; or when the scales are equal, we
can perform a comparison of the two decimals themselves without extracting the
first digit.
We can _probably_ also make this computation less costly by using
{{BigDecimal.scaleByPowerOfTen}}, if we keep it.
What do you think?
> Arithmetic operators over decimal truncate results
> --
>
> Key: CASSANDRA-15232
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15232
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL/Semantics
>Reporter: Benedict
>Assignee: Liudmila Kornilova
>Priority: Normal
> Labels: pull-request-available
> Fix For: 4.0
>
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> The decimal operators hard-code a 128 bit precision for their computations.
> Probably a precision needs to be configured or decided somehow, but it’s not
> clear why 128bit was chosen. Particularly for multiplication and addition,
> it’s very unclear why we truncate, which is different to our behaviour for
> e.g. sum() aggregates. Probably for division we should also ensure that we
> do not reduce the precision of the two operands. A minimum of decimal128
> seems reasonable, but a maximum does not.
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15232) Arithmetic operators over decimal truncate results
[
https://issues.apache.org/jira/browse/CASSANDRA-15232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16891664#comment-16891664
]
Benjamin Lerer commented on CASSANDRA-15232:
Regarding,
{quote} it’s not clear why 128bit was chosen. {quote}
and
{quote} Particularly for multiplication and addition, it’s very unclear why we
truncate,{quote}
I can add some clarity. It is some mistakes from my part. :-(
> Arithmetic operators over decimal truncate results
> --
>
> Key: CASSANDRA-15232
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15232
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL/Semantics
>Reporter: Benedict
>Assignee: Liudmila Kornilova
>Priority: Normal
> Labels: pull-request-available
> Fix For: 4.0
>
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> The decimal operators hard-code a 128 bit precision for their computations.
> Probably a precision needs to be configured or decided somehow, but it’s not
> clear why 128bit was chosen. Particularly for multiplication and addition,
> it’s very unclear why we truncate, which is different to our behaviour for
> e.g. sum() aggregates. Probably for division we should also ensure that we
> do not reduce the precision of the two operands. A minimum of decimal128
> seems reasonable, but a maximum does not.
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15232) Arithmetic operators over decimal truncate results
[ https://issues.apache.org/jira/browse/CASSANDRA-15232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16891475#comment-16891475 ] Benedict commented on CASSANDRA-15232: -- Thanks [~Override] for the patch. I'll take a closer look as soon as I have the chance, hopefully within the next week or so. > Arithmetic operators over decimal truncate results > -- > > Key: CASSANDRA-15232 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15232 > Project: Cassandra > Issue Type: Improvement > Components: CQL/Semantics >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 4.0 > > Time Spent: 1h 10m > Remaining Estimate: 0h > > The decimal operators hard-code a 128 bit precision for their computations. > Probably a precision needs to be configured or decided somehow, but it’s not > clear why 128bit was chosen. Particularly for multiplication and addition, > it’s very unclear why we truncate, which is different to our behaviour for > e.g. sum() aggregates. Probably for division we should also ensure that we > do not reduce the precision of the two operands. A minimum of decimal128 > seems reasonable, but a maximum does not. -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15232) Arithmetic operators over decimal truncate results
[ https://issues.apache.org/jira/browse/CASSANDRA-15232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16890801#comment-16890801 ] Liudmila Kornilova commented on CASSANDRA-15232: This is how postgres calculates scale https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/numeric.c#L7800 Before this commit Cassandra used to limit precision off all operations to 34 (maximum number of significant digits) After this commit: * add, multiply and subtract operations limit precision to 1 digits * add, multiply and subtract trim trailing zeros, so operations do not affect scale/precision of next operations * division sets minimum scale to 32. So 1000 / 3 produces 333. (32 digits after dot) * division also tries to guess the position of first significant digit in result (this idea is taken from postgres) so 0.1 / 3 will produce 5+32=37 digits after dot, 32 of them will be significant * division sets maximum scale to 1000 (the constant is also taken from postgres) * division will not use scale that is lower that scale of any operands The PR is not 100% ready because mod operation needs to be implemented > Arithmetic operators over decimal truncate results > -- > > Key: CASSANDRA-15232 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15232 > Project: Cassandra > Issue Type: Improvement > Components: CQL/Semantics >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 4.0 > > Time Spent: 10m > Remaining Estimate: 0h > > The decimal operators hard-code a 128 bit precision for their computations. > Probably a precision needs to be configured or decided somehow, but it’s not > clear why 128bit was chosen. Particularly for multiplication and addition, > it’s very unclear why we truncate, which is different to our behaviour for > e.g. sum() aggregates. Probably for division we should also ensure that we > do not reduce the precision of the two operands. A minimum of decimal128 > seems reasonable, but a maximum does not. -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15232) Arithmetic operators over decimal truncate results
[ https://issues.apache.org/jira/browse/CASSANDRA-15232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16886740#comment-16886740 ] Benedict commented on CASSANDRA-15232: -- I looked into this a while ago, and I believe Postgres uses the maximum precision of the two operands, which seems like a reasonable approach. > Arithmetic operators over decimal truncate results > -- > > Key: CASSANDRA-15232 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15232 > Project: Cassandra > Issue Type: Bug >Reporter: Benedict >Priority: Normal > > The decimal operators hard-code a 128 bit precision for their computations. > Probably a precision needs to be configured or decided somehow, but it’s not > clear why 128bit was chosen. Particularly for multiplication and addition, > it’s very unclear why we truncate, which is different to our behaviour for > e.g. sum() aggregates. Probably for division we should also ensure that we > do not reduce the precision of the two operands. A minimum of decimal128 > seems reasonable, but a maximum does not. -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
