You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by vladimirtkach <gi...@git.apache.org> on 2018/02/16 14:41:35 UTC
[GitHub] drill pull request #1123: DRILL-6158: NaN, Infinity issues
GitHub user vladimirtkach opened a pull request:
https://github.com/apache/drill/pull/1123
DRILL-6158: NaN, Infinity issues
- changed comparison rules for NaN, Infinity values. For now NaN is the biggest value, Infinity - second biggest value
- fixed min, max, trunc functions for NaN, Infinity values
- made drill use original sqrt function instead of substitution
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vladimirtkach/drill DRILL-6154
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1123.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1123
----
commit 6195546c58604c833c8e9134227a353884401e24
Author: vladimir tkach <vo...@...>
Date: 2018-02-16T14:36:49Z
DRILL-6158: NaN, Infinity issues
- changed comparison rules for NaN, Infinity values. For now NaN is the biggest value, Infinity - second biggest value
- fixed min, max, trunc functions for NaN, Infinity values
- made drill use original sqrt function instead of substitution
----
---
[GitHub] drill pull request #1123: DRILL-6154: NaN, Infinity issues
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1123
---
[GitHub] drill pull request #1123: DRILL-6154: NaN, Infinity issues
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1123#discussion_r168777666
--- Diff: exec/java-exec/src/main/codegen/templates/AggrTypeFunctions1.java ---
@@ -102,7 +102,20 @@ public void add() {
</#if>
nonNullCount.value = 1;
<#if aggrtype.funcName == "min">
- value.value = Math.min(value.value, in.value);
+ <#if type.inputType?contains("Float4") || type.inputType?contains("Float8")>
--- End diff --
1. Please mind original indent.
2. Please add comment describing how we chose min / max for nan / inf values.
3. Please consider using if without nested if: `if - elseif - else`.
---
[GitHub] drill pull request #1123: DRILL-6154: NaN, Infinity issues
Posted by vladimirtkach <gi...@git.apache.org>.
Github user vladimirtkach commented on a diff in the pull request:
https://github.com/apache/drill/pull/1123#discussion_r168918725
--- Diff: exec/java-exec/src/main/codegen/templates/AggrTypeFunctions1.java ---
@@ -102,7 +102,20 @@ public void add() {
</#if>
nonNullCount.value = 1;
<#if aggrtype.funcName == "min">
- value.value = Math.min(value.value, in.value);
+ <#if type.inputType?contains("Float4") || type.inputType?contains("Float8")>
--- End diff --
@arina-ielchiieva code is updated.
---
[GitHub] drill issue #1123: DRILL-6154: NaN, Infinity issues
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1123
+1, LGTM.
---
[GitHub] drill pull request #1123: DRILL-6154: NaN, Infinity issues
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1123#discussion_r168777217
--- Diff: exec/java-exec/src/main/codegen/templates/MathFunctions.java ---
@@ -67,7 +67,11 @@ private GMathFunctions(){}
public static class ${func.className}${type.input} implements DrillSimpleFunc {
@Param ${type.input}Holder in;
+ <#if func.funcName == 'sqrt'>
+ @Output Float8Holder out;
--- End diff --
Please add comment why we use float holder for sqrt function.
---
[GitHub] drill pull request #1123: DRILL-6154: NaN, Infinity issues
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1123#discussion_r168777355
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillConvertletTable.java ---
@@ -34,10 +40,17 @@
public static HashMap<SqlOperator, SqlRexConvertlet> map = new HashMap<>();
public static SqlRexConvertletTable INSTANCE = new DrillConvertletTable();
+ private static SqlRexConvertlet sqrtConvertlet = new SqlRexConvertlet() {
--- End diff --
Please add comment explaining why we need sqrt specific convertlet.
---