IGNITE-20311 Sql. Fix behaviour of ROUND function. #2690

lowka · 2023-10-16T08:51:43Z

Fixes behaviour of ROUND function.

https://issues.apache.org/jira/browse/IGNITE-20311

Thank you for submitting the pull request.

To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:

The Review Checklist

Formal criteria: TC status, codestyle, mandatory documentation. Also make sure to complete the following:
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Design: new code conforms with the design principles of the components it is added to.
Patch quality: patch cannot be split into smaller pieces, its size must be reasonable.
Code quality: code is clean and readable, necessary developer documentation is added if needed.
Tests code quality: test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.

Notes

Apache Ignite Coding Guidelines

zstan · 2023-10-18T12:13:51Z

...s/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDataTypesTest.java

@@ -322,7 +322,7 @@ public void testDecimalLiteral() {
                "SELECT DECIMAL '0.09'  BETWEEN DECIMAL '0.06' AND DECIMAL '0.07'")
                .returns(false).check();

-        assertQuery("SELECT ROUND(DECIMAL '10.000', 2)").returns(new BigDecimal("10.00")).check();
+        assertQuery("SELECT ROUND(DECIMAL '10.000', 2)").returns(new BigDecimal("10.000")).check();


probably we need to add more accurate test:
assertQuery("SELECT TYPEOF(ROUND(DECIMAL '10.000', 2))").returns("DECIMAL(5, 3)").check();
wdyt ?

honestly i forgot about our discussion for such a case, believe that type need to be the same in such a case too.

This PR adds test cases for ROUND function to ItFunctionsTest. Test cases with return type assertions can be found there.

...les/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/RexImpTable.java

korlov42 · 2023-10-18T14:13:29Z

...les/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/RexImpTable.java

@@ -4359,6 +4364,26 @@ private static class DefaultImplementor extends AbstractRexCallImplementor {
    }
  }

+  private static class IgniteMethodNameImplementor extends AbstractRexCallImplementor {


why do we need new implementor?

Because defineMethod expects that all methods to be defined on calcite's SqlFunctions. (1)
And it is not possible to define a method with multiple overloads in IgniteSqlFunctions. (2)

The second one can be fixed if we to allow defining functions by name w/o specifying their signatures e.g. ROUND function would look like:

ROUND("round");

This would also require adding methodName to IgniteSqlFunctions

i did my best ))) to align RexImpTable with calcite and one more change (

Because defineMethod expects that all methods to be defined on calcite's SqlFunctions

that's not true, you could supply reference to particular java.lang.reflect.Method. In that case you can use static method from any class

And it is not possible to define a method with multiple overloads in IgniteSqlFunctions

why do you can define multiple overloads in SqlFunctions, but not in IgniteSqlFunctions?

korlov42 · 2023-10-18T14:16:59Z

...ine/src/test/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctionsTest.java

+
+    /** Tests for ROUND(x) function. */
+    @Test
+    public void testRound() {


let's add all possible numeric types to the test cases. The same for another variant of this function

There only 4 variants of this function - int, long, double and BigDecimal. shorts and byte are promoted to ints by calcite.
I can add tests for all numeric types at the SQL level.

int, long, double and BigDecimal. shorts and byte are promoted to ints by calcite.

why do we need int version then? 🤔

Because ROUND(1) has type ROUND(<int>).
if there is no ROUND(<int>) overload, then SELECT ROUND(1) is going to rejected by the validator.

Have you tried to delete this overload and run test?

korlov42 · 2023-10-18T14:22:48Z

modules/runner/src/integrationTest/sql/types/decimal/test_decimal_ops.test

BTW, we have sql/function/numeric/test_round.test. Let's add test cases to this file to cover all possible types

korlov42 · 2023-10-18T14:26:57Z

...ine/src/test/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctionsTest.java

+    /** Tests for ROUND(x, s) function, where x is a BigDecimal value. */
+    @ParameterizedTest
+    @CsvSource({
+            "1.123, 0, 1.000",


can we have similar set of tests for every type? LIke, tests with negative s

Sure. Done.

Fixes after review.

Add test cases.

Update test cases.

korlov42 · 2023-10-19T07:47:32Z

...-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java

+
+    /** SQL {@code ROUND} operator applied to int values. */
+    public static int sround(int b0) {
+        return sround(BigDecimal.valueOf(b0)).intValue();


can we avoid creating BigDecimal for primitive types?

Update test cases.

xtern · 2023-10-19T08:36:01Z

modules/runner/src/integrationTest/java/org/apache/ignite/internal/sqllogic/ItSqlLogicTest.java

@@ -140,7 +140,7 @@
 @Tag(value = "sqllogic")
 @ExtendWith(SystemPropertiesExtension.class)
 @WithSystemProperty(key = "IMPLICIT_PK_ENABLED", value = "true")
-@SqlLogicTestEnvironment(scriptsRoot = "src/integrationTest/sql")
+@SqlLogicTestEnvironment(scriptsRoot = "src/integrationTest/sql", regex = "test_round")


Don't forget to revert this.
Also unmute/rewrite TODOs related to ticket in ItSqlOperatorsTest#testMathFunctions

Thanks. That was committed by mistake.

Fix lookup.

Fix.

Fix rounding behaviour.

check style fixes.

lowka · 2023-10-19T09:58:08Z

...-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java

@@ -434,4 +503,31 @@ public boolean rolledUpColumnValidInsideAgg(String column, SqlCall call,
            return true;
        }
    }
+
+    private static long divide(long p, long q, RoundingMode mode) {


We need HALF_UP for ROUND function and HALF_DOWN for TRUNCATE.

check style fixes.

Update test cases. Fix typo.

korlov42 · 2023-10-20T13:05:16Z

...les/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/RexImpTable.java

@@ -1145,6 +1146,11 @@ private void defineMethod(SqlOperator operator, Method method,
      map.put(operator, new MethodImplementor(method, nullPolicy, false));
    }

+    private void defineIgniteMethod(SqlOperator operator, String method,


I think, this can be deleted

korlov42 · 2023-10-20T13:07:31Z

modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteMethod.java

+    IgniteMethod(Class<?> clazz, String methodName, boolean overloadedMethod) {
+        if (overloadedMethod) {
+            // Allow calcite to select appropriate method at a call site.
+            this.method = Arrays.stream(IgniteSqlFunctions.class.getMethods())


that's kinda strange decision: you have particular class as input argument, yet you are using IgniteSqlFunctions.class

That was a typo. Thanks, fixed it.

korlov42 · 2023-10-20T13:15:42Z

...s/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItFunctionsTest.java

@@ -344,6 +351,193 @@ public void testSubstr() {
        assertThrowsWithCause(() -> sql("SELECT SUBSTR('1234567', 1, -3)"), IgniteException.class, "negative substring length");
    }

+    /** Test cases for ROUND function that accepts integral numeric types. */
+    @TestFactory
+    public Stream<DynamicTest> testRoundIntegral() {


please rewrite these tests without TestFactory. You don't need TestFactory until you are working on a test framework.

Rewrite tests.

korlov42 · 2023-10-23T11:36:47Z

...-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java

+    // SQL ROUND function
+
+    /** SQL {@code ROUND} operator applied to int values. */
+    public static int sround(int b0) {


although such set of overrides are working correctly for every numeric type, ExpressionFactory can't generate optimal code. Try to change tests in order to use dynamic params rather constants, and print out generated code of expressions to see difference

add byte/short versions of sround.

* IGNITE-19425 Use data nodes from DistributionZoneManager instead of BaselineManager#nodes and remove BaselineManager (apache#2605) * IGNITE-20127 Implement delayed replication acks - Fixes apache#2520. Signed-off-by: Alexey Scherbakov <alexey.scherbakoff@gmail.com> * IGNITE-20330 Create an abstraction for building indexes (apache#2631) * IGNITE-20513 Use NativeType as the type of the system view column (apache#2668) * IGNITE-20590 Remove TxManager.beginImplicit() (apache#2669) * IGNITE-20588 Use consistent schema when converting data in KV/record views (apache#2667) * IGNITE-20342 Rollback transaction for SQL execution issues (apache#2611) * IGNITE-20445 Clean up write intents for RW transaction on replication… (apache#2657) * IGNITE-20607 Move NativeType to core (apache#2670) * IGNITE-20484 NPE when some operation occurs when the primary replica … (apache#2672) * IGNITE-20442 Sql. Extend grammar with transaction related statements. (apache#2663) * IGNITE-20620 Add index availability command to catalog (apache#2680) * IGNITE-20521 Split Security API from ignite-security module (apache#2662) * IGNITE-20621 Add a replication group index build completion listener to IndexBuilder (apache#2682) * IGNITE-19226 Fetch table schema by timestamp (apache#2648) * IGNITE-19325 Unmute test MultiActorPlacementDriverTest#prolongAfterActiveActorChanger (apache#2677) * IGNITE-20625 Fix ODBC metadata reading error (apache#2687) * IGNITE-20457 Verify commitTimestamp against enlisted partitions expiration timestamps (apache#2658) * IGNITE-20635 Cleanup code wrt IGNITE-18733 mentions (apache#2686) * IGNITE-20317 Return metastorage invokes for zones changes in handlers, immediately recalculate data nodes when scale up/down is immediate. (apache#2685) * IGNITE-19276 Implement a mechanism to build indices distributively (apache#2676) * IGNITE-20567 Move the 'enabled' flag from the authentication configuration to security (apache#2665) * IGNITE-20578 Implement scan over system view (apache#2678) * IGNITE-20387: Remap most exceptions to SqlExceptions for SQL API (apache#2613) * IGNITE-19217 Implement foreign keys query for ODBC (apache#2692) * IGNITE-20512 Remove port range from HTTP server (apache#2673) * Remove port range from HTTP server configuration * Update junit5 version to avoid jar hell. * IGNITE-20657 At the checkpoint ArrayIndexOutOfBoundsException (apache#2696) * IGNITE-20444 Sql. Add restrictions for execution tx related statements with single statement mode (apache#2683) * IGNITE-20116 Linearize storage updates with safeTime adjustment rules (apache#2689) * IGNITE-20636 Add to the MakeIndexAvailableCommand the ability to use only the indexId (apache#2691) * IGNITE-19219 Implement primary keys query in ODBC (apache#2699) * IGNITE-20366 testBatchReadPutConcurrently failed (apache#2694) * IGNITE-20435 Preserve key order in batch opperations (deleteAll, deleteAllExact, insertAll) (apache#2664) * IGNITE-20385 Sql. Fixed handling of NodeLeftException (apache#2622) * IGNITE-20672 Broke compilation (apache#2705) * IGNITE-20530 Start building indexes for write-only indexes (apache#2704) * IGNITE-20659 Placement driver do not create a lease (apache#2700) * IGNITE-20668 Increase wait after a DDL to account for idle safe-time propagation period (apache#2703) * IGNITE-20522 Create the default user 'Ignite' (apache#2684) * Ignite/ignite default user is added to the configuration if the user did not specify one * security.authentication.enabled -> security.enabled * We didn't have the ability to set dynamic defaults in the current implementation of configuration before this patch. Moreover, we don't have the ability to set a default value for NamedConfigValue at all. Now you can add the code to the configuration module if you want to add some defaults based on user-provided values or other external conditions. * IGNITE-20616 ensureReplicaIsPrimary should use getPrimaryReplica instead of awaitPrimaryReplica (apache#2701) * IGNITE-20671 Sql. Fixed ItSqlApiTest#ddl test (apache#2708) * IGNITE-20677 Sql. Improve error reporting in assertThrowsSqlException (apache#2707) * IGNITE-20600 Sql. Fix a message of an error, which occurs while updating primary key column (apache#2695) * IGNITE-20478 Sql. Get rid of UNSPECIFIED_VALUE_PLACEHOLDER (apache#2671) * IGNITE-20430 Got rid of unused set and fixed replica waiters removal (apache#2604) * IGNITE-20454 Sql. Added a callback that is notified when data prefetching is complete (apache#2674) * IGNITE-20002 Implement durable unlock on primary partition re-election (apache#2697) * IGNITE-20630 Select only available nodes for deployment unit download (apache#2713) * IGNITE-20693 Fixed NPE in placement driver actor on deactivation (apache#2718) * IGNITE-20695 Cleanup resource (apache#2723) * IGNITE-20545 Improve logging in AbstractRpcTest (apache#2717) * IGNITE-20395 Clean up write intents for RW transaction on primary (apache#2679) * IGNITE-20699 Decrease idle safe time propagation period in tests (apache#2730) * IGNTIE-20629 Exclude ODBC build from assemble pipeline by default (apache#2706) * IGNITE-20706 Rename CatalogSchemaManager to SchemaManager (apache#2733) * IGNITE-20702 Fix NPE in ReplicaManager.onReplicaMessageReceived (apache#2731) * IGNITE-20704 Add methods to fsync files and directories (apache#2735) * IGNITE-20703 LeaseUpdaterTest causes a NullPointerException (apache#2732) * IGNITE-20599 Implement 'NOT_TOMBSTONE' operation in the meta storage dsl. (apache#2722) * IGNITE-20042 Check table existence before executing each operation in an RW transaction (apache#2721) * IGNITE-20576 Fix testGetReturningTupleWithUnknownSchemaRequestsNewSchema (apache#2738) * IGNITE-20359 Expose storage profiles as a node attribute (apache#2711) * IGNITE-20439 Sql. Support multiple schemas in CatalogSqlSchemaManager (apache#2719) * IGNITE-20311 Sql. Fix behaviour of ROUND function (apache#2690) * IGNITE-20684 Use proper sync method (apache#2715) * IGNITE-20670 Extract deployment code integration test to related module (apache#2714) * IGNITE-20304 Sql. Documentation for system views module (apache#2726) * IGNITE-20644 Java thin: Fix error detection in doSchemaOutInOpAsync (apache#2739) Fix well-known error detection in `ClientTable` when `sendServerExceptionStackTraceToClient` is enabled: `unwrapRootCause` did not work, the important exception can be on any level. * IGNITE-20498 Fix potential catalog version order violations (apache#2734) * IGNITE-20560 Remove the field out of sync with the map (apache#2660) * IGNITE-20099 Update okhttp version (apache#2746) * IGNITE-20720 Move ClusterPerTestIntegrationTest and ClusterPerClassIntegrationTest to Test Fixtures (apache#2744) * IGNITE-20618 Sql. Degradation of SELECT operations performance over time (apache#2728) * IGNITE-20726: Fix incorrect link that mentions a closed ticket. (apache#2745) * IGNITE-20727 Pass schema version in each read/write ReplicaRequest (apache#2747) * IGNITE-20561 Change condition for DistributionZonesUtil#triggerKeyConditionForZonesChanges to solve inconsistency issues (apache#2743) * IGNITE-20734 Fixed a link to cli doc (apache#2750) * IGNITE-20739 fix compilation after merge of IGNITE-20561 (apache#2753) * IGNITE-20624 Fix race between getting logical topology and mapping fragments (apache#2710) --------- Signed-off-by: Alexey Scherbakov <alexey.scherbakoff@gmail.com> Co-authored-by: Mirza Aliev <alievmirza@gmail.com> Co-authored-by: Alexey Scherbakov <alexey.scherbakoff@gmail.com> Co-authored-by: Kirill Tkalenko <tkalkirill@yandex.ru> Co-authored-by: korlov42 <korlov@gridgain.com> Co-authored-by: Roman Puchkovskiy <roman.puchkovskiy@gmail.com> Co-authored-by: Max Zhuravkov <shhwwa@gmail.com> Co-authored-by: Cyrill <cyrill.sizov@gmail.com> Co-authored-by: Ivan Gagarkin <gagarkin.iiu@gmail.com> Co-authored-by: Vladislav Pyatkov <vldpyatkov@gmail.com> Co-authored-by: Igor Sapego <isapego@apache.org> Co-authored-by: Alexander Lapin <lapin1702@gmail.com> Co-authored-by: ygerzhedovich <41903880+ygerzhedovich@users.noreply.github.com> Co-authored-by: Aleksandr Pakhomov <apkhmv@gmail.com> Co-authored-by: Pavel Pereslegin <xxtern@gmail.com> Co-authored-by: Evgeniy Stanilovskiy <stanilovsky@gmail.com> Co-authored-by: Denis Chudov <moonglloom@gmail.com> Co-authored-by: Mikhail <Pochatkin@users.noreply.github.com> Co-authored-by: Vadim Pakhnushev <8614891+valepakh@users.noreply.github.com> Co-authored-by: Alexander Polovtcev <alex.polovtcev@gmail.com> Co-authored-by: Pavel Tupitsyn <ptupitsyn@apache.org> Co-authored-by: Andrew V. Mashenkov <AMashenkov@users.noreply.github.com> Co-authored-by: IgGusev <igusev@gridgain.com>

[IGNITE-20311] Sql. Fix behaviour of ROUND function.

d3a4d29

lowka force-pushed the IGNITE-20311 branch from 123b027 to df68592 Compare October 16, 2023 12:18

IGNITE-20311 Sql. Fix behaviour of ROUND function.

1c58334

lowka force-pushed the IGNITE-20311 branch 4 times, most recently from b8308cf to be996b7 Compare October 17, 2023 12:14

lowka marked this pull request as ready for review October 18, 2023 03:07

IGNITE-20311 Sql. Fix behaviour of ROUND function.

dcbbdc9

lowka force-pushed the IGNITE-20311 branch from be996b7 to dcbbdc9 Compare October 18, 2023 03:16

lowka added 2 commits October 18, 2023 07:59

Merge branch 'main' into IGNITE-20311_fix_round_func

1696c59

Merge branch 'main' into IGNITE-20311_fix_round_func

d33db9b

zstan reviewed Oct 18, 2023

View reviewed changes

zstan approved these changes Oct 18, 2023

View reviewed changes

korlov42 reviewed Oct 18, 2023

View reviewed changes

lowka added 7 commits October 18, 2023 21:10

IGNITE-20311 Sql. Fix behaviour of ROUND function.

4c1739d

Fixes after review.

IGNITE-20311 Sql. Fix behaviour of ROUND function.

e209294

Add test cases.

IGNITE-20311 Sql. Fix behaviour of ROUND function.

205282c

Add test cases.

IGNITE-20311 Sql. Fix behaviour of ROUND function.

9a3c6b6

Update test cases.

IGNITE-20311 Sql. Fix behaviour of ROUND function.

586ec90

Update test cases.

IGNITE-20311 Sql. Fix behaviour of ROUND function.

5e7de61

Update test cases.

IGNITE-20311 Sql. Fix behaviour of ROUND function.

7a2059e

Update test cases.

korlov42 reviewed Oct 19, 2023

View reviewed changes

IGNITE-20311 Sql. Fix behaviour of ROUND function.

b738633

Update test cases.

lowka requested a review from korlov42 October 19, 2023 07:49

xtern reviewed Oct 19, 2023

View reviewed changes

lowka added 4 commits October 19, 2023 12:10

IGNITE-20311 Sql. Fix behaviour of ROUND function.

1280553

Fix lookup.

IGNITE-20311 Sql. Fix behaviour of ROUND function.

81df5cd

Fix.

IGNITE-20311 Sql. Fix behaviour of ROUND function.

3dde3ee

Fix rounding behaviour.

IGNITE-20311 Sql. Fix behaviour of ROUND function.

a27fd3a

Fix rounding behaviour.

lowka added 2 commits October 19, 2023 12:48

IGNITE-20311 Sql. Fix behaviour of ROUND function.

cb7e7b8

Fix rounding behaviour.

IGNITE-20311 Sql. Fix behaviour of ROUND function.

ca608e1

check style fixes.

lowka commented Oct 19, 2023

View reviewed changes

lowka added 2 commits October 19, 2023 15:39

IGNITE-20311 Sql. Fix behaviour of ROUND function.

081e9ab

check style fixes.

IGNITE-20311 Sql. Fix behaviour of ROUND function.

7a947d3

Update test cases. Fix typo.

korlov42 reviewed Oct 20, 2023

View reviewed changes

lowka added 4 commits October 20, 2023 17:51

IGNITE-20311 Sql. Fix behaviour of ROUND function.

7a1c12f

Rewrite tests.

IGNITE-20311 Sql. Fix behaviour of ROUND function.

c9c06ed

Rewrite tests.

Merge branch 'main' into IGNITE-20311_round

83ba522

Merge branch 'main' into IGNITE-20311

701c4fd

korlov42 reviewed Oct 23, 2023

View reviewed changes

IGNITE-20311 Sql. Fix behaviour of ROUND function.

a5a505f

add byte/short versions of sround.

korlov42 merged commit 51bbcf0 into apache:main Oct 23, 2023
1 check passed

korlov42 deleted the IGNITE-20311 branch October 23, 2023 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-20311 Sql. Fix behaviour of ROUND function. #2690

IGNITE-20311 Sql. Fix behaviour of ROUND function. #2690

lowka commented Oct 16, 2023 •

edited

zstan Oct 18, 2023

zstan Oct 18, 2023

lowka Oct 19, 2023

korlov42 Oct 18, 2023

lowka Oct 18, 2023 •

edited

zstan Oct 19, 2023

korlov42 Oct 19, 2023

korlov42 Oct 18, 2023

lowka Oct 18, 2023 •

edited

korlov42 Oct 19, 2023

lowka Oct 19, 2023 •

edited

korlov42 Oct 19, 2023

lowka Oct 19, 2023

korlov42 Oct 18, 2023

korlov42 Oct 18, 2023

lowka Oct 19, 2023

korlov42 Oct 19, 2023

lowka Oct 19, 2023

xtern Oct 19, 2023

lowka Oct 19, 2023

lowka Oct 19, 2023

korlov42 Oct 20, 2023

lowka Oct 20, 2023

korlov42 Oct 20, 2023

lowka Oct 20, 2023

korlov42 Oct 20, 2023

lowka Oct 20, 2023

korlov42 Oct 23, 2023

IGNITE-20311 Sql. Fix behaviour of ROUND function. #2690

IGNITE-20311 Sql. Fix behaviour of ROUND function. #2690

Conversation

lowka commented Oct 16, 2023 • edited

The Review Checklist

Notes

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

lowka Oct 18, 2023 • edited

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

lowka Oct 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowka Oct 19, 2023 • edited

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowka commented Oct 16, 2023 •

edited

lowka Oct 18, 2023 •

edited

lowka Oct 18, 2023 •

edited

lowka Oct 19, 2023 •

edited