You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org> on 2018/02/15 23:47:41 UTC
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9346
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Before this patch, the output type of round() ceil() floor() trunc() was
not always the same as the input type. It was also inconsistent in
general. For example, round(double) returned an integer, but
round(double, int) returned a double.
After looking at other database systems, we decided that the guideline
should be that the output type should be the same as the input type. In
this patch, we change the behavior of the previously mentioned functions
so that if a double is given then a double is returned.
We also modify the rounding behavior to always round away from zero.
Before, we were rounding towards positive infinity in some cases.
Testinging:
- Updated tests
- Ran an exhaustive build which passed.
Cherry-picks: not for 2.x
Change-Id: I77541678012edab70b182378b11ca8753be53f97
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/math-functions.h
M be/src/exprs/scalar-fn-call.cc
M common/function-registry/impala_functions.py
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
7 files changed, 77 insertions(+), 70 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/9346/1
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@121
PS1, Line 121: v.val * pow(10.0, scale.val) + ((v.val < 0) ? -0.5 : 0.5)) / pow(10.0, scale.val));
Should we also check for overflows here , set FunctionContext with an error and return a NULL?
ctx->SetError
Found a couple of possible inconsistencies (without your change). If your change fixes it we can add these tests?
[nightly-unsecure-2.gce.cloudera.com:21000] > select dround(cast(2.500001 as double), 256);
Query: select dround(cast(2.500001 as double), 256)
Query submitted at: 2018-03-02 16:44:05 (Coordinator: http://nightly-unsecure-2.gce.cloudera.com:25000)
Query progress can be monitored at: http://nightly-unsecure-2.gce.cloudera.com:25000/query_plan?query_id=c5421271ce74b797:4da8702100000000
+---------------------------------------+
| dround(cast(2.500001 as double), 256) |
+---------------------------------------+
| 2.500001 |
+---------------------------------------+
Fetched 1 row(s) in 0.01s
[nightly-unsecure-2.gce.cloudera.com:21000] > select dround(cast(2.500001 as double), 257);
Query: select dround(cast(2.500001 as double), 257)
Query submitted at: 2018-03-02 16:44:09 (Coordinator: http://nightly-unsecure-2.gce.cloudera.com:25000)
Query progress can be monitored at: http://nightly-unsecure-2.gce.cloudera.com:25000/query_plan?query_id=d4969d969dba34a:639969f00000000
+---------------------------------------+
| dround(cast(2.500001 as double), 257) |
+---------------------------------------+
| 2.500001000000001 |
+---------------------------------------+
Fetched 1 row(s) in 0.01s
[nightly-unsecure-2.gce.cloudera.com:21000] > select dround(cast(2.500001 as double), 400);
Query: select dround(cast(2.500001 as double), 400)
Query submitted at: 2018-03-02 16:50:01 (Coordinator: http://nightly-unsecure-2.gce.cloudera.com:25000)
Query progress can be monitored at: http://nightly-unsecure-2.gce.cloudera.com:25000/query_plan?query_id=c741e669098c64a0:d7514ce100000000
+---------------------------------------+
| dround(cast(2.500001 as double), 400) |
+---------------------------------------+
| NaN |
+---------------------------------------+
Fetched 1 row(s) in 0.01s
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Sat, 03 Mar 2018 00:58:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Before this patch, the output type of round() ceil() floor() trunc() was
not always the same as the input type. It was also inconsistent in
general. For example, round(double) returned an integer, but
round(double, int) returned a double.
After looking at other database systems, we decided that the guideline
should be that the output type should be the same as the input type. In
this patch, we change the behavior of the previously mentioned functions
so that if a double is given then a double is returned.
We also modify the rounding behavior to always round away from zero.
Before, we were rounding towards positive infinity in some cases.
Testinging:
- Updated tests
- Ran an exhaustive build which passed.
Cherry-picks: not for 2.x
Change-Id: I77541678012edab70b182378b11ca8753be53f97
Reviewed-on: http://gerrit.cloudera.org:8080/9346
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/math-functions.h
M be/src/exprs/scalar-fn-call.cc
M common/function-registry/impala_functions.py
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
7 files changed, 110 insertions(+), 68 deletions(-)
Approvals:
Alex Behm: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 5
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
Patch Set 1:
impalad crashes with this patch at startup.
I0302 16:29:52.235250 19443 status.cc:125] Unable to find _ZN6impala13MathFunctions9RoundUpToEPN10impala_udf15FunctionContextERKNS1_9DoubleValERKNS1_9BigIntValE
dlerror: /home/anuj/Impala/be/build/latest/service/impalad: undefined symbol: _ZN6impala13MathFunctions9RoundUpToEPN10impala_udf15FunctionContextERKNS1_9DoubleValERKNS1_9BigIntValE
@ 0x166cd33 impala::Status::Status()
@ 0x1a664ec impala::DynamicLookup()
@ 0x182e579 impala::LibCache::GetSoFunctionPtr()
@ 0x182ef5a impala::LibCache::CheckSymbolExists()
@ 0x193a533 ResolveSymbolLookup()
@ 0x193b00b Java_org_apache_impala_service_FeSupport_NativeLookupSymbol
@ 0x7fb3508edc39 (unknown)
F0302 16:29:52.236079 19443 frontend.cc:108] java.lang.ExceptionInInitializerError
at org.apache.impala.service.BackendConfig.create(BackendConfig.java:47)
at org.apache.impala.service.JniFrontend.<init>(JniFrontend.java:123)
Caused by: java.lang.RuntimeException: Builtin symbol 'impala::MathFunctions::RoundUpTo'[DOUBLE, BIGINT] not found!
at org.apache.impala.catalog.ScalarFunction.createBuiltin(ScalarFunction.java:82)
at org.apache.impala.catalog.Db.addScalarBuiltin(Db.java:393)
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Sat, 03 Mar 2018 00:35:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Before this patch, the output type of round() ceil() floor() trunc() was
not always the same as the input type. It was also inconsistent in
general. For example, round(double) returned an integer, but
round(double, int) returned a double.
After looking at other database systems, we decided that the guideline
should be that the output type should be the same as the input type. In
this patch, we change the behavior of the previously mentioned functions
so that if a double is given then a double is returned.
We also modify the rounding behavior to always round away from zero.
Before, we were rounding towards positive infinity in some cases.
Testinging:
- Updated tests
- Ran an exhaustive build which passed.
Cherry-picks: not for 2.x
Change-Id: I77541678012edab70b182378b11ca8753be53f97
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/math-functions.h
M be/src/exprs/scalar-fn-call.cc
M common/function-registry/impala_functions.py
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
7 files changed, 110 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/9346/4
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Mar 2018 04:43:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2170/
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Mar 2018 00:56:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Before this patch, the output type of round() ceil() floor() trunc() was
not always the same as the input type. It was also inconsistent in
general. For example, round(double) returned an integer, but
round(double, int) returned a double.
After looking at other database systems, we decided that the guideline
should be that the output type should be the same as the input type. In
this patch, we change the behavior of the previously mentioned functions
so that if a double is given then a double is returned.
We also modify the rounding behavior to always round away from zero.
Before, we were rounding towards positive infinity in some cases.
Testinging:
- Updated tests
- Ran an exhaustive build which passed.
Cherry-picks: not for 2.x
Change-Id: I77541678012edab70b182378b11ca8753be53f97
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/math-functions.h
M be/src/exprs/scalar-fn-call.cc
M common/function-registry/impala_functions.py
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
7 files changed, 107 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/9346/2
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@a107
PS1, Line 107:
Wow this was so wrong, thanks for fixing.
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@120
PS1, Line 120: return DoubleVal(trunc(
Do we have a test that shows the better behavior for trunc() versus the previous floor()?
http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@266
PS1, Line 266: [['sign'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Sign'],
Should we also have a DECIMAL version?
http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@280
PS1, Line 280: [['ceil', 'ceiling', 'dceil'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Ceil'],
Don't we also need FLOAT versions of these if our goal is to return the same output type as input type?
Fine to make these additions in a separate patch if you prefer. Maybe there are more functions that need additional signatures.
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Mar 2018 18:18:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
Patch Set 4: Code-Review+1
I'm happy with this change. I think a BE person should +2.
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Mar 2018 00:47:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@120
PS1, Line 120: return DoubleVal(trunc(
> Do we have a test that shows the better behavior for trunc() versus the pre
pow(10.0, scale.val) is likely expensive to compute and it isn't clear that the compiler will know the function is "pure". Should we instead do a table lookup and bounds check to make sure the scale parameter is sane?
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@121
PS1, Line 121: v.val * pow(10.0, scale.val) + ((v.val < 0) ? -0.5 : 0.5)) / pow(10.0, scale.val));
> Should we also check for overflows here , set FunctionContext with an error
Can you undo the code movement here so that the change is more clear?
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions.h
File be/src/exprs/math-functions.h:
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions.h@80
PS1, Line 80: static DoubleVal RoundUpTo(FunctionContext*, const DoubleVal&, const BigIntVal&);
Why the ordering change? Also, why allow BigIntVal range here, isn't that just asking for trouble with overflow?
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Mar 2018 20:16:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@120
PS1, Line 120: return DoubleVal(trunc(
> pow(10.0, scale.val) is likely expensive to compute and it isn't clear that
Done. According to my benchmark, this implementation is much faster (over 10x) than the old one (for cases where scale is less than 20).
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@120
PS1, Line 120: return DoubleVal(trunc(
> Do we have a test that shows the better behavior for trunc() versus the pre
Yes, Alex, there is a test in expr-test.cc on line 5465
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@121
PS1, Line 121: v.val * pow(10.0, scale.val) + ((v.val < 0) ? -0.5 : 0.5)) / pow(10.0, scale.val));
> Can you undo the code movement here so that the change is more clear?
Done
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@121
PS1, Line 121: v.val * pow(10.0, scale.val) + ((v.val < 0) ? -0.5 : 0.5)) / pow(10.0, scale.val));
> Should we also check for overflows here , set FunctionContext with an error
Anuj, the problem here is with double arithmetic. In general, it's not possible to represent a double number precisely. Which is why it is weird to do rounding and specify the number of digits to round to in decimal. I don't think that anything can be done about this.
This is exactly the reason why people should be using the decimal type.
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions.h
File be/src/exprs/math-functions.h:
http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions.h@80
PS1, Line 80: static DoubleVal RoundUpTo(FunctionContext*, const DoubleVal&, const BigIntVal&);
> Why the ordering change? Also, why allow BigIntVal range here, isn't that
Fixed the ordering.
It makes sense to use bigintval because if someone passes in a smallint, it can be implicitly cast to bigint, but not the other way around. So bigint is kind of like an integer wildcard. Which overflow are you talking about?
http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@266
PS1, Line 266: [['sign'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Sign'],
> Should we also have a DECIMAL version?
Do you think we would need to have a sign() function for double, float, int, bigint, decimal, etc then?
http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@280
PS1, Line 280: [['ceil', 'ceiling', 'dceil'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Ceil'],
> Don't we also need FLOAT versions of these if our goal is to return the sam
I agree, this should be addressed in a separate patch.
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 21:51:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Mar 2018 00:55:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9346/2/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/9346/2/be/src/exprs/math-functions-ir.cc@152
PS2, Line 152: // TODO: should we support non-constant seed?
> I think this will DCHECK if a negative scale is passed
oops, fixed.
http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@266
PS1, Line 266: [['sign'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Sign'],
> I think this is a question for Greg. It's mostly about whether this functio
Agreed.
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Mar 2018 22:37:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns
......................................................................
Patch Set 2:
(2 comments)
I'm pretty happy with this change. Let's not forget the follow-on work for those other functions (if necessary, as determined by Greg)
http://gerrit.cloudera.org:8080/#/c/9346/2/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/9346/2/be/src/exprs/math-functions-ir.cc@152
PS2, Line 152: return DoubleVal(trunc(
I think this will DCHECK if a negative scale is passed
http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:
http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@266
PS1, Line 266: [['sign'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Sign'],
> Do you think we would need to have a sign() function for double, float, int
I think this is a question for Greg. It's mostly about whether this function should follow the same philosophy of "input type == output type". I suggest we defer this to another discussion/patch.
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Mon, 19 Mar 2018 20:41:55 +0000
Gerrit-HasComments: Yes