Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.4.0-incubating
    • Component/s: None
    • Labels:

      Description

      avg() on BigDecimal column hits SqlFunctions.divide(BigDeciaml, BigDecimal), which further calls BigDecimal.divide(BigDecimal). This method will throw exception when the result has infinite digits after the decimal point, e.g. 10/9=1.11111...

      The stack is as follows:

      java.lang.ArithmeticException: Non-terminating decimal expansion; no exact representable decimal result.
      	at java.math.BigDecimal.divide(BigDecimal.java:1690)
      	at org.apache.calcite.runtime.SqlFunctions.divide(SqlFunctions.java:547)
      	at Baz$5$1.current(Unknown Source)
      	at org.apache.calcite.linq4j.Linq4j$EnumeratorIterator.next(Linq4j.java:685)
      	at org.apache.calcite.avatica.util.IteratorCursor.next(IteratorCursor.java:48)
      	at org.apache.calcite.avatica.AvaticaResultSet.next(AvaticaResultSet.java:215)
      	at sqlline.IncrementalRows.hasNext(IncrementalRows.java:62)
      	at sqlline.TableOutputFormat.print(TableOutputFormat.java:33)
      	at sqlline.SqlLine.print(SqlLine.java:1652)
      	at sqlline.Commands.execute(Commands.java:833)
      	at sqlline.Commands.sql(Commands.java:732)
      	at sqlline.SqlLine.dispatch(SqlLine.java:807)
      	at sqlline.SqlLine.begin(SqlLine.java:681)
      	at sqlline.SqlLine.start(SqlLine.java:398)
      	at sqlline.SqlLine.main(SqlLine.java:292)
      

      Alternatively, divide(BigDecimal divisor, MathContext mc) should be called instead, so BigDecimal knows the precision and rounding mode to handle the infinite number.

      ---------------- Imported from GitHub ----------------
      Url: https://github.com/julianhyde/optiq/issues/280
      Created by: liyang-gmt8
      Labels:
      Created at: Mon May 12 10:25:07 CEST 2014
      State: open

        Activity

        Hide
        github-import GitHub Import added a comment -

        [Date: Mon May 12 18:32:48 CEST 2014, Author: vlsi]

        Can you please clarify your view on the desired default behavior? (except from 'do not crash')

        I incline to the `MathContext` being specified via connection properties with some reasonable defaults (e.g. comparable with `double` accuracy)

        Show
        github-import GitHub Import added a comment - [Date: Mon May 12 18:32:48 CEST 2014, Author: vlsi ] Can you please clarify your view on the desired default behavior? (except from 'do not crash') I incline to the `MathContext` being specified via connection properties with some reasonable defaults (e.g. comparable with `double` accuracy)
        Hide
        github-import GitHub Import added a comment -

        [Date: Mon May 12 21:34:44 CEST 2014, Author: julianhyde]

        BigDecimal is slow and uses a lot of memory compared to other computation methods, e.g. implementing decimal using a shifted long. Aggregate functions such as `avg` should only use BigDecimal if the user has given us an indication that they really want to use it. Otherwise Optiq should convert the values to `double` or `long` at the start.

        But when we do use BigDecimal, I agree with @vlsi that we should use a MathContext configured according to connection properties. `SqlFunctions.divide(BigDecimal, BigDecimal)` would get an extra `MathContext` parameter, and we'd change the code-generator accordingly. Likewise other functions in `SqlFunctions` that use `BigDecimal`.

        Show
        github-import GitHub Import added a comment - [Date: Mon May 12 21:34:44 CEST 2014, Author: julianhyde ] BigDecimal is slow and uses a lot of memory compared to other computation methods, e.g. implementing decimal using a shifted long. Aggregate functions such as `avg` should only use BigDecimal if the user has given us an indication that they really want to use it. Otherwise Optiq should convert the values to `double` or `long` at the start. But when we do use BigDecimal, I agree with @vlsi that we should use a MathContext configured according to connection properties. `SqlFunctions.divide(BigDecimal, BigDecimal)` would get an extra `MathContext` parameter, and we'd change the code-generator accordingly. Likewise other functions in `SqlFunctions` that use `BigDecimal`.
        Hide
        github-import GitHub Import added a comment -

        [Date: Tue May 13 03:47:12 CEST 2014, Author: liyang-gmt8]

        Agree. Connection properties with reasonable defaults sounds perfect.

        Show
        github-import GitHub Import added a comment - [Date: Tue May 13 03:47:12 CEST 2014, Author: liyang-gmt8 ] Agree. Connection properties with reasonable defaults sounds perfect.
        Hide
        julianhyde Julian Hyde added a comment -

        Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/e39911ed.

        I did not add a configuration parameter. I hard-coded DECIMAL64. If that is not good enough open another case.

        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/e39911ed . I did not add a configuration parameter. I hard-coded DECIMAL64. If that is not good enough open another case.
        Hide
        jnadeau Jacques Nadeau added a comment -

        Resolved in release 1.4.0-incubating (2015-08-23)

        Show
        jnadeau Jacques Nadeau added a comment - Resolved in release 1.4.0-incubating (2015-08-23)

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            github-import GitHub Import
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development