Skip to content

Better error message for unsupported double values#11409

Merged
abhishekagarwal87 merged 3 commits intoapache:masterfrom
abhishekagarwal87:nan-infinity
Jul 8, 2021
Merged

Better error message for unsupported double values#11409
abhishekagarwal87 merged 3 commits intoapache:masterfrom
abhishekagarwal87:nan-infinity

Conversation

@abhishekagarwal87
Copy link
Copy Markdown
Contributor

@abhishekagarwal87 abhishekagarwal87 commented Jul 5, 2021

A constant expression may evaluate to Double.NEGATIVE_INFINITY/Double.POSITIVE_INFINITY/Double.NAN e.g. log10(0). When using such an expression in native queries, the user will get the corresponding value without any error. In SQL, however, the user will run into NumberFormatException because we convert the double to big-decimal while constructing a literal numeric expression. This probably should be fixed in calcite - see https://issues.apache.org/jira/browse/CALCITE-2067. This PR adds a verbose error message so that users can take corrective action without scratching their heads.


Key changed/added classes in this PR
  • DruidRexExecutor

This PR has:

  • been self-reviewed.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.

if (Double.isNaN(exprResultDouble))
{
String expression = druidExpression.getExpression();
throw new IAE("'%s' evaluates to 'NaN' that is not supported. You can either cast the expression as bigint ('cast(%s as bigint)') or char ('cast(%s as char)') or change the expression itself",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

wondering if using a NumberFormatException instead of IAE makes more sense here.

if (Double.isInfinite(exprResultDouble))
{
String expression = druidExpression.getExpression();
throw new IAE("'%s' evaluates to '+/-Infinity' that is not supported. You can either cast the expression as bigint ('cast(%s as bigint)') or char ('cast(%s as char)') or change the expression itself",
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.

I think that toString for exprResultDouble does evaluate as +/-INFINITY or NaN - so maybe that can be directly used when Double.isNaN(exprResultDouble) || Double.isInfinite(exprResultDouble)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍 good tip.

Copy link
Copy Markdown
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

👍

Comment thread sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java Outdated
@abhishekagarwal87 abhishekagarwal87 merged commit 3481bb0 into apache:master Jul 8, 2021
jihoonson pushed a commit to jihoonson/druid that referenced this pull request Jul 12, 2021
A constant expression may evaluate to Double.NEGATIVE_INFINITY/Double.POSITIVE_INFINITY/Double.NAN e.g. log10(0). When using such an expression in native queries, the user will get the corresponding value without any error. In SQL, however, the user will run into NumberFormatException because we convert the double to big-decimal while constructing a literal numeric expression. This probably should be fixed in calcite - see https://issues.apache.org/jira/browse/CALCITE-2067. This PR adds a verbose error message so that users can take corrective action without scratching their heads.
@abhishekagarwal87 abhishekagarwal87 deleted the nan-infinity branch July 22, 2021 10:13
@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
@clintropolis clintropolis mentioned this pull request Apr 12, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants