Skip to content

[CALCITE-7826] IS NOT DISTINCT FROM gives wrong result in Enumerable convention when applied to DECIMAL values of different scales - #5296

Closed
julianhyde wants to merge 2 commits into
apache:mainfrom
julianhyde:7826-distinct-from-decimal
Closed

julianhyde wants to merge 2 commits into
apache:mainfrom
julianhyde:7826-distinct-from-decimal

Conversation

@julianhyde

Copy link
Copy Markdown
Contributor

CALCITE-7826

The branch has two commits: a reproducer, and a fix. I'll squash after review.

…in Enumerable convention when applied to DECIMAL values of different scales

In the Enumerable convention, IS NOT DISTINCT FROM and IS DISTINCT
FROM are implemented by RexImpTable.DistinctFromImplementor, which
compares non-null operands using Objects.equals and does not
harmonize the operand types. BigDecimal.equals is sensitive to scale,
so 1.10 (DECIMAL(3, 2)) and 1.1 (DECIMAL(2, 1)) are distinct, whereas
'=' compares them numerically and finds them equal. The test
evaluates both operators, and '=', on those two values, and gets:

  a=1.10; b=1.1; eq=true; indf=false; idf=true

rather than the correct:

  a=1.10; b=1.1; eq=true; indf=true; idf=false

A query parsed from SQL is not affected, because
StandardConvertletTable.convertIsDistinctFrom expands IS [NOT]
DISTINCT FROM (using RelOptUtil.isDistinctFrom) into IS NULL and '='
on operands that have been cast to a common type. RelBuilder
.isNotDistinctFrom expands it in the same way. So the test calls the
operators directly, using RelBuilder.call, as a plan built by an
application may do. (The problem was found in Morel, which does
this.)
…convention when applied to DECIMAL values of different scales

Fix the problem reproduced by the previous commit.

RexImpTable.DistinctFromImplementor, which implements IS NOT DISTINCT
FROM and IS DISTINCT FROM in the Enumerable convention, compared two
non-null operands using Objects.equals. For BigDecimal values that is
sensitive to scale, so 1.10 and 1.1 were distinct.

Now, if both operands are BigDecimal values, it compares them using
SqlFunctions.eq(BigDecimal, BigDecimal) (via the new
BuiltInMethod.EQ_DECIMAL), which the '=' operator also uses, and
which ignores scale. Other values are still compared using
Objects.equals.

(Harmonizing the operand types, as other implementors do, would not
fix the problem: both operands already have Java class BigDecimal,
and the conversion does not change their scale.)
@sonarqubecloud

Copy link
Copy Markdown

@xiedeyantu xiedeyantu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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

Successfully merging this pull request may close these issues.

3 participants