Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: make decimal literals actual decimals #4378

Merged

Conversation

big-andy-coates
Copy link
Contributor

@big-andy-coates big-andy-coates commented Jan 24, 2020

Description

Fixes build failures in #4364.

Previously, DecimalLiteral was the only literal type where the contained java type did not reflect the literal type. DoubleLiteral has a double field, StringLiteral a String field, IntegerLiteral an int field, etc. But DecimalLiteral contained a String field.

While this works with the legacy code for persistent and push queries, (which tend to 'toString' everything), this is actually causing issues with pull queries, which pass the result of Literal.getValue to the SqlValueCoercer. The coercer will happily coerce an INT, BIGINT or DECIMAL to a DOUBLE. But it won't convert a STRING to a DOUBLE. Because DecimalLiteral.getValue was returning a String the coercion failed. This issue only comes to light now that we can have ROWKEY of type DOUBLE. This issue causes the following RQTT pull query test to fail:

{
      "name": "windowed single key lookup - DOUBLE",
      "statements": [
        "CREATE STREAM INPUT (ROWKEY DOUBLE KEY, IGNORED INT) WITH (kafka_topic='test_topic', value_format='JSON');",
        "CREATE TABLE AGGREGATE AS SELECT COUNT(1) AS COUNT FROM INPUT WINDOW TUMBLING(SIZE 1 SECOND) GROUP BY ROWKEY;",
        "SELECT * FROM AGGREGATE WHERE ROWKEY=10.1;",
        "SELECT * FROM AGGREGATE WHERE ROWKEY=0;"
      ],
      "inputs": [
        {"topic": "test_topic", "timestamp": 12346, "key": 11.1, "value": {"val": 1}}
      ],
      "responses": [
        {"admin": {"@type": "currentStatus"}},
        {"admin": {"@type": "currentStatus"}},
        {"query": [
          {"header":{"schema":"`ROWKEY` DOUBLE KEY, `WINDOWSTART` BIGINT KEY, `ROWTIME` BIGINT, `COUNT` BIGINT"}},
          {"row":{"columns":[10.1, 12000, 12345, 1]}}
        ]}
      ]
    }

It fails with an error complaining the value "10.1" can not be coerced to the type of ROWKEY, which is DOUBLE.

Switching DecimalLiteral to actually contain a BigDecimal fixes this issue.

Testing done

usual

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

Previously, `DecimalLiteral` was the only literal type where the contained java type did not reflect the literal type. `DoubleLiteral` has a `double` field, `StringLiteral` a `String` field, `IntegerLiteral` an `int` field, etc.  But `DecimalLiteral` contained a `String` field.

While this works with the legacy code for persistent and push queries, (which tend to 'toString' everything), this is actually causing issues with pull queries, which pass the result of `Literal.getValue` to the `SqlValueCoercer`.  The coercer will happily coerce an `INT`, `BIGINT` or `DECIMAL` to a `DOUBLE`. But it won't convert a `STRING` to a `DOUBLE`. Because `DecimalLiteral.getValue` was returning a `String` the coercion failed.  This issue only comes to light now that we can have `ROWKEY` of type `DOUBLE`.  This issue causes the following RQTT pull query test to fail:

```json
{
      "name": "windowed single key lookup - DOUBLE",
      "statements": [
        "CREATE STREAM INPUT (ROWKEY DOUBLE KEY, IGNORED INT) WITH (kafka_topic='test_topic', value_format='JSON');",
        "CREATE TABLE AGGREGATE AS SELECT COUNT(1) AS COUNT FROM INPUT WINDOW TUMBLING(SIZE 1 SECOND) GROUP BY ROWKEY;",
        "SELECT * FROM AGGREGATE WHERE ROWKEY=10.1;",
        "SELECT * FROM AGGREGATE WHERE ROWKEY=0;"
      ],
      "inputs": [
        {"topic": "test_topic", "timestamp": 12346, "key": 11.1, "value": {"val": 1}}
      ],
      "responses": [
        {"admin": {"@type": "currentStatus"}},
        {"admin": {"@type": "currentStatus"}},
        {"query": [
          {"header":{"schema":"`ROWKEY` DOUBLE KEY, `WINDOWSTART` BIGINT KEY, `ROWTIME` BIGINT, `COUNT` BIGINT"}},
          {"row":{"columns":[10.1, 12000, 12345, 1]}}
        ]}
      ]
    }
```

It fails with an error complaining the value `"10.1"` can not be coerced to the type of `ROWKEY`, which is `DOUBLE`.

Switching `DecimalLiteral` to actually contain a `BigDecimal` fixes this issue.
@big-andy-coates big-andy-coates requested a review from a team as a code owner January 24, 2020 17:46
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

I originally had it like this and then decided against it in favor of storing a String, now I can't remember why... If I figure it out later I'll revert this :)

@big-andy-coates
Copy link
Contributor Author

I originally had it like this and then decided against it in favor of storing a String, now I can't remember why... If I figure it out later I'll revert this :)

Cool - though you'll then have to 'fix' pull queries by hacking them.

@big-andy-coates big-andy-coates merged commit 2cbf6f0 into confluentinc:master Jan 25, 2020
@big-andy-coates big-andy-coates deleted the decimal_decimals branch January 25, 2020 22:40
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.

2 participants