[FLINK-23614][table-planner] The resulting scale of TRUNCATE(DECIMAL,… #16740

paul8263 · 2021-08-06T10:14:07Z

… ...) is not correct

What is the purpose of the change

Fixe the issue that the resulting scale of TRUNCATE(DECIMAL, ...) is not correct.

Brief change log

flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/MathFunctionsITCase.java
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/sql/FlinkSqlOperatorTable.java

Verifying this change

This change added tests and can be verified as follows:

Added truncate decimal test case in flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/MathFunctionsITCase.java

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API, i.e., is any changed class annotated with @Public(Evolving): no
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? no

flinkbot · 2021-08-06T10:17:55Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit aceb2a2 (Fri Aug 06 10:17:54 UTC 2021)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

Airblader · 2021-08-06T10:30:36Z

...lanner/src/main/java/org/apache/flink/table/planner/functions/sql/FlinkSqlOperatorTable.java

+            new SqlFunction(
+                    "TRUNCATE",
+                    SqlKind.OTHER_FUNCTION,
+                    FlinkReturnTypes.ROUND_FUNCTION_NULLABLE,


If we're reusing this strategy, the name doesn't seem appropriate anymore. We should rename this to something that explains what it's doing.

If you look into this method you'll find that it is calling LogicalTypeMerging#findRoundDecimalType. In that method there is a line stating that

// NOTE: rounding may increase the digits by 1, therefore we need +1 on precisions. return new DecimalType(false, 1 + precision - scale + round, round);

However for truncate function the number of digits will not increase, thus FlinkReturnTypes.ROUND_FUNCTION_NULLABLE is not the best choice to use here.

What I'll suggest is that you create a new method in LogicalTypeMerging called findTruncateDecimalType. This method and findRoundDecimalType can together reuse some code. Then you might want to create FlinkReturnTypes.TRUNCATE_FUNCTION_NULLABLE which will also reuse a lot of code with ROUND_FUNCTION_NULLABLE.

According to the second review request below, if round and truncate could use the same logic, could I refactor those method name to things like 'findRoundOrTruncateDecimalType'?

I would prefer "ROUNDING_NULLABLE", as "rounding" is a more general behavior (we can round down or round up), not specified to a function.

OK. I'll keep it the same as the round method.

flinkbot · 2021-08-06T10:49:52Z

CI report:

4a1e653 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

tsreaper · 2021-08-09T02:44:38Z

...lanner/src/main/java/org/apache/flink/table/planner/functions/sql/FlinkSqlOperatorTable.java

+            new SqlFunction(
+                    "TRUNCATE",
+                    SqlKind.OTHER_FUNCTION,
+                    FlinkReturnTypes.ROUND_FUNCTION_NULLABLE,


If you look into this method you'll find that it is calling LogicalTypeMerging#findRoundDecimalType. In that method there is a line stating that

// NOTE: rounding may increase the digits by 1, therefore we need +1 on precisions. return new DecimalType(false, 1 + precision - scale + round, round);

However for truncate function the number of digits will not increase, thus FlinkReturnTypes.ROUND_FUNCTION_NULLABLE is not the best choice to use here.

What I'll suggest is that you create a new method in LogicalTypeMerging called findTruncateDecimalType. This method and findRoundDecimalType can together reuse some code. Then you might want to create FlinkReturnTypes.TRUNCATE_FUNCTION_NULLABLE which will also reuse a lot of code with ROUND_FUNCTION_NULLABLE.

tsreaper · 2021-08-09T02:49:57Z

...able-planner/src/test/java/org/apache/flink/table/planner/functions/MathFunctionsITCase.java

+                                DataTypes.DECIMAL(8, 2).notNull()),
+                TestSpec.forFunction(BuiltInFunctionDefinitions.TRUNCATE)
+                        .onFieldsWithData(new BigDecimal("123.456"))
+                        // TRUNCATE(DECIMAL(6, 3) NOT NULL, 2) => DECIMAL(6, 2) NOT NULL
+                        .testResult(
+                                $("f0").truncate(2),
+                                "TRUNCATE(f0, 2)",
+                                new BigDecimal("123.45"),
+                                DataTypes.DECIMAL(6, 2).notNull()));


This MathFunctionITCase, as stated in the java docs, is for BuiltInFunctionDefinitions. If you follow the usage of BuiltinFunctionDefinitions you'll see that it is converted to FlinkSqlOperatorTable.TRUNCATE. For Flink SQL scalar functions we always add tests in ScalarFunctionsTest. Please add your tests there.

A thorough test for a function should include all data types it supported as well as their corresponding null values. The tests in ScalarFunctionTest#testTruncate may not be complete so please complete them with all supported data types. See FlinkSqlOperatorTable.TRUNCATE to get all its supported types.

Hi @tsreaper ,
Thank you for your advice.
In flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/SqlFunctionUtils.java there is an implementation of truncating method designed for DecimalData:

public static DecimalData struncate(DecimalData b0, int b1) { if (b1 >= b0.scale()) { return b0; } BigDecimal b2 = b0.toBigDecimal() .movePointRight(b1) .setScale(0, RoundingMode.DOWN) .movePointLeft(b1); int p = b0.precision(); int s = b0.scale(); if (b1 < 0) { return DecimalData.fromBigDecimal(b2, Math.min(38, 1 + p - s), 0); } else { return DecimalData.fromBigDecimal(b2, 1 + p - s + b1, b1); } }

It uses the same logic as the round method.

After I thought over this issue, I suggest that we should add 1 on precision (same logic as round method). If we did not do that, for example, given a number f1 0.333 with the type of DECIMAL(3, 3), if we call truncate(f1, 0), the precision of the result would be 0, which would trigger an exception of 'Decimal precision must be between 1 and 38 (both inclusive)'.

Those info above is my point of view. If you have any suggestion, please comment below. Thanks a lot.

Nice catch. With this in mind we shall keep using FlinkReturnTypes.ROUND_FUNCTION_NULLABLE instead of modifying it. Still the test cases should be moved to the appropriate place. You can also add this special "truncate to zero" test case in your test.

Hi @tsreaper ,
I added several "truncated to zero" test cases.

paul8263 · 2021-08-10T08:05:28Z

@flinkbot run azure

tsreaper · 2021-08-11T03:08:39Z

I've checked this issue again and found that adding tests to ScalarFunctionsTest.scala is not enough. This is because function test use the generated function code directly and will not check for the return type of the function. So you may also need to add some tests in org.apache.flink.table.planner.runtime.batch.sql.CalcITCase.

Also there are CI failures. Please keep an eye on the comment of @flinkbot for the results. If there are CI failures then the PR will never be merged.

tsreaper

Looks good to me. @JingsongLi any other thoughts?

tsreaper · 2021-08-17T05:05:48Z

Hi @paul8263 , this PR is conflicting with the master branch. Please resolve the conflicts.

paul8263 · 2021-08-18T02:34:22Z

Hi @tsreaper ,
I solved the conflicts in 9296b4e.
When I ran the unit tests I encountered an error: org.apache.flink.changelog.fs.FsStateChangelogStorageFactory in org.apache.flink.changelog.fs is not public, cannot access it from external packages.
In FLINK-23279, org.apache.flink.changelog.fs.FsStateChangelogStorageFactory was added in StreamFaultToleranceTestBase.java. It seems that changes in other commits might lead to the test failure.

… ...) is not correct

JingsongLi

Looks good to me!

… is not correct This closes #16740 (cherry picked from commit e55e664)

… is not correct This closes apache#16740

rmetzger added the review=description? label Aug 6, 2021

Airblader reviewed Aug 6, 2021

View reviewed changes

rmetzger added the component=TableSQL/Runtime label Aug 6, 2021

paul8263 force-pushed the FLINK-23614 branch from aceb2a2 to c084abd Compare August 9, 2021 01:22

tsreaper requested changes Aug 9, 2021

View reviewed changes

paul8263 force-pushed the FLINK-23614 branch from c084abd to ae3ae37 Compare August 10, 2021 08:00

paul8263 force-pushed the FLINK-23614 branch 2 times, most recently from d7e70d6 to 56b3375 Compare August 12, 2021 06:21

tsreaper approved these changes Aug 13, 2021

View reviewed changes

paul8263 force-pushed the FLINK-23614 branch from 56b3375 to 9296b4e Compare August 18, 2021 02:17

[FLINK-23614][table-planner] The resulting scale of TRUNCATE(DECIMAL,…

4a1e653

… ...) is not correct

JingsongLi force-pushed the FLINK-23614 branch from 9296b4e to 4a1e653 Compare September 7, 2021 04:41

JingsongLi self-requested a review September 7, 2021 04:43

JingsongLi approved these changes Sep 7, 2021

View reviewed changes

JingsongLi merged commit e55e664 into apache:master Sep 7, 2021

godfreyhe pushed a commit that referenced this pull request Oct 29, 2021

[FLINK-23614][table-planner] The resulting scale of TRUNCATE(DECIMAL)…

8c7cbcd

… is not correct This closes #16740 (cherry picked from commit e55e664)

niklassemmler pushed a commit to niklassemmler/flink that referenced this pull request Feb 3, 2022

[FLINK-23614][table-planner] The resulting scale of TRUNCATE(DECIMAL)…

ceaf701

… is not correct This closes apache#16740

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-23614][table-planner] The resulting scale of TRUNCATE(DECIMAL,… #16740

[FLINK-23614][table-planner] The resulting scale of TRUNCATE(DECIMAL,… #16740

paul8263 commented Aug 6, 2021

flinkbot commented Aug 6, 2021

Airblader Aug 6, 2021

tsreaper Aug 9, 2021

paul8263 Aug 10, 2021

tsreaper Aug 10, 2021 •

edited

paul8263 Aug 10, 2021

flinkbot commented Aug 6, 2021 •

edited

tsreaper Aug 9, 2021

tsreaper Aug 9, 2021

paul8263 Aug 10, 2021

tsreaper Aug 10, 2021

paul8263 Aug 10, 2021

paul8263 commented Aug 10, 2021

tsreaper commented Aug 11, 2021

tsreaper left a comment

tsreaper commented Aug 17, 2021

paul8263 commented Aug 18, 2021

JingsongLi left a comment

[FLINK-23614][table-planner] The resulting scale of TRUNCATE(DECIMAL,… #16740

[FLINK-23614][table-planner] The resulting scale of TRUNCATE(DECIMAL,… #16740

Conversation

paul8263 commented Aug 6, 2021

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Aug 6, 2021

Automated Checks

Review Progress

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsreaper Aug 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flinkbot commented Aug 6, 2021 • edited

CI report:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul8263 commented Aug 10, 2021

tsreaper commented Aug 11, 2021

tsreaper left a comment

Choose a reason for hiding this comment

tsreaper commented Aug 17, 2021

paul8263 commented Aug 18, 2021

JingsongLi left a comment

Choose a reason for hiding this comment

tsreaper Aug 10, 2021 •

edited

flinkbot commented Aug 6, 2021 •

edited