[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2020: Add rounding when casting from decimal to int
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5951/3/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:

PS3, Line 297: round ? dv.ToIntVal(scale) : 
to_type(dv.whole_part(scale));
perhaps pass 'round' to ToInt() and move the "else" block in there since it's 
basically the truncation case. that way we're more symmetric.

and then the to_type can stay here, and instead ToInt() can return the 
primitive (like whole_part() does), which would keep all the conversions 
between DecimalValue and udf::*IntVal types within this code, which I think 
makes more sense since this code is the UDF builtin code, whereas DecimalValue 
is the decimal slot representation code internal to impala.


http://gerrit.cloudera.org:8080/#/c/5951/3/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

Line 88: return RESULT_T::null();
> That's a good idea.  Should I still be returning raw UDF types (in which ca
I would probably leave the UDF type (e.g. BigIntVal) out of here and just 
return the raw primitive, unless using the UDF type simplifies things for some 
reason.


-- 
To view, visit http://gerrit.cloudera.org:8080/5951
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int

[Zach Amsden (Code Review)]

Zach Amsden has posted comments on this change.

Change subject: IMPALA-2020: Add rounding when casting from decimal to int
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5951/3/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

Line 84:   auto divisor = DecimalUtil::GetScaleMultiplier(scale);
> doesn't round, of course.
Done.  Turns out you can do this without branching.  I think right now I have a 
branch but I can get rid of it.


Line 88: return RESULT_T::null();
> how about sticking with the convention used in the rest of this file where 
That's a good idea.  Should I still be returning raw UDF types (in which case I 
need to return the null version of the type).


-- 
To view, visit http://gerrit.cloudera.org:8080/5951
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2020: Add rounding when casting from decimal to int
..


Patch Set 3:

> > I'm not opposed to cleaning up the AnyVal stuff like that, but
 > > given that udf.h stuff dictates UDF compatibility, it's not
 > > completely trivial. It doesn't look like it would break binary
 > > compatibility though. But, in case something goes wrong, how
 > about
 > > we do that as a separate change so it could be backed out without
 > > affecting the decimal work? It doesn't look like the decimal
 > stuff
 > > will depend on it, right?
 > 
 > No, but it gets a lot cleaner to test the limits by giving the
 > generic form an underlying type.  I deliberately did not change
 > FloatVal, since the equality operator is currently kind of broken. 
 > Everything else should be binary compatible.

But you can get that by looking at *Val::val, no?

Another thing to watch out for is that since users write and compile their own 
UDFs, we've had trouble in the past with accidently increasing the C++ version 
required to build these things. I don't remember the specific problems (maybe 
Michael or Tim do) and also don't know whether that's a real issue with this 
change.

-- 
To view, visit http://gerrit.cloudera.org:8080/5951
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int

[Zach Amsden (Code Review)]

Zach Amsden has posted comments on this change.

Change subject: IMPALA-2020: Add rounding when casting from decimal to int
..


Patch Set 3:

> I'm not opposed to cleaning up the AnyVal stuff like that, but
 > given that udf.h stuff dictates UDF compatibility, it's not
 > completely trivial. It doesn't look like it would break binary
 > compatibility though. But, in case something goes wrong, how about
 > we do that as a separate change so it could be backed out without
 > affecting the decimal work? It doesn't look like the decimal stuff
 > will depend on it, right?

No, but it gets a lot cleaner to test the limits by giving the generic form an 
underlying type.  I deliberately did not change FloatVal, since the equality 
operator is currently kind of broken.  Everything else should be binary 
compatible.

-- 
To view, visit http://gerrit.cloudera.org:8080/5951
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int

[Zach Amsden (Code Review)]

Zach Amsden has uploaded a new patch set (#3).

Change subject: IMPALA-2020: Add rounding when casting from decimal to int
..

IMPALA-2020: Add rounding when casting from decimal to int

Preview diff, not working yet, mostly to see if I can successfully
push diffs against my ASF Impala fork, and also to get early
feedback on the UDF change.  Update - pushing from fork didn't
work.

Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/runtime/decimal-value.h
M be/src/runtime/decimal-value.inline.h
M be/src/udf/udf.h
4 files changed, 50 insertions(+), 108 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/5951/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5951
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 

[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2020: Add rounding when casting from decimal to int
..


Patch Set 2:

I'm not opposed to cleaning up the AnyVal stuff like that, but given that udf.h 
stuff dictates UDF compatibility, it's not completely trivial. It doesn't look 
like it would break binary compatibility though. But, in case something goes 
wrong, how about we do that as a separate change so it could be backed out 
without affecting the decimal work? It doesn't look like the decimal stuff will 
depend on it, right?

-- 
To view, visit http://gerrit.cloudera.org:8080/5951
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int

[Zach Amsden (Code Review)]

Zach Amsden has uploaded a new patch set (#2).

Change subject: IMPALA-2020: Add rounding when casting from decimal to int
..

IMPALA-2020: Add rounding when casting from decimal to int

Preview diff, not working yet, mostly to see if I can successfully
push diffs against my ASF Impala fork, and also to get early
feedback on the UDF change.  Update - pushing from fork didn't
work.

Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/runtime/decimal-value.h
M be/src/runtime/decimal-value.inline.h
M be/src/udf/udf.h
4 files changed, 41 insertions(+), 109 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/5951/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5951
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden