[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int
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 has posted comments on this change. Change subject: IMPALA-2020: Add rounding when casting from decimal to int .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/5951/4/be/src/udf/udf.h File be/src/udf/udf.h: Line 437: // XXX Null floats only compare equal if value is also equal? I'm filing a JIRA for this. -- 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: 4 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 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
Zach Amsden has uploaded a new patch set (#4). Change subject: IMPALA-2020: Add rounding when casting from decimal to int .. IMPALA-2020: Add rounding when casting from decimal to int First implementation of rounding mode on CAST from decimal to int. Testing: In progress. Expect a full test suite for both modes and all edge cases to be covered. 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, 57 insertions(+), 113 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/5951/4 -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Zach Amsden
[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int
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/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. Line 88: return RESULT_T::null(); how about sticking with the convention used in the rest of this file where the caller decides what to do with overflow by returning via *overflow? The reason is because in the not to distant future, we'll want to raise an error on overflow rather than silently convert to null. -- 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 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 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 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 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 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
[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int
Zach Amsden has uploaded a new change for review. http://gerrit.cloudera.org:8080/5951 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, 37 insertions(+), 105 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/5951/1 -- To view, visit http://gerrit.cloudera.org:8080/5951 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden