IMPALA-7473: fix crash when printing decimal with -v=3 The bug is that gcc emitted an aligned load instruction to the int128 value_, and we didn't exercise that code path in our testing. The fix is to use memcpy() to force an unaligned load.
Also fix a NullPointerException in the frontend encountered when running some tests with -v=3. Testing: Added a unit test that crashed when run with a release build. Change-Id: I772681b78cda27d6c2086b91151ca8bdcd901828 Reviewed-on: http://gerrit.cloudera.org:8080/12029 Reviewed-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Project: http://git-wip-us.apache.org/repos/asf/impala/repo Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/9af2fdc4 Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/9af2fdc4 Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/9af2fdc4 Branch: refs/heads/master Commit: 9af2fdc44a06d6309b21cbca1cbc6a3f383c9b81 Parents: 0164fa7 Author: Tim Armstrong <tarmstr...@cloudera.com> Authored: Thu Nov 29 17:46:52 2018 -0800 Committer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Committed: Thu Dec 6 05:42:20 2018 +0000 ---------------------------------------------------------------------- be/src/runtime/decimal-test.cc | 20 ++++++++++++++++++++ be/src/runtime/decimal-value.inline.h | 14 +++++++++----- .../org/apache/impala/planner/PlanNode.java | 12 ++++++++---- 3 files changed, 37 insertions(+), 9 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/impala/blob/9af2fdc4/be/src/runtime/decimal-test.cc ---------------------------------------------------------------------- diff --git a/be/src/runtime/decimal-test.cc b/be/src/runtime/decimal-test.cc index 25d2f65..ac9fea7 100644 --- a/be/src/runtime/decimal-test.cc +++ b/be/src/runtime/decimal-test.cc @@ -19,9 +19,11 @@ #include <stdio.h> #include <iostream> #include <limits> +#include <sstream> #include <boost/cstdint.hpp> #include <boost/lexical_cast.hpp> #include "runtime/decimal-value.inline.h" +#include "runtime/raw-value.h" #include "runtime/types.h" #include "testutil/gtest-util.h" #include "util/string-parser.h" @@ -708,6 +710,24 @@ TEST(DecimalTest, MultiplyScaleOverflow) { EXPECT_FALSE(overflow); } +// Test that unaligned decimal values are handled correctly. +TEST(DecimalTest, UnalignedValues) { + // Regression test for IMPALA-7473 that triggered a crash in release builds. + Decimal16Value aligned(12345); + uint8_t* unaligned_mem = reinterpret_cast<uint8_t*>(malloc(sizeof(Decimal16Value) + 9)); + memcpy(&unaligned_mem[9], &aligned, sizeof(aligned)); + Decimal16Value* unaligned = reinterpret_cast<Decimal16Value*>(&unaligned_mem[9]); + // VerifyToString() worked even prior to the bugfix because GCC happened to generate + // code without aligned load instructions. + VerifyToString(*unaligned, 28, 2, "123.45"); + // PrintValue() contained different generated code that wasn't safe for unaligned + // values. + stringstream ss; + RawValue::PrintValue(unaligned, ColumnType::CreateDecimalType(28, 2), 0, &ss); + EXPECT_EQ("123.45", ss.str()); + free(unaligned_mem); +} + enum Op { ADD, SUBTRACT, http://git-wip-us.apache.org/repos/asf/impala/blob/9af2fdc4/be/src/runtime/decimal-value.inline.h ---------------------------------------------------------------------- diff --git a/be/src/runtime/decimal-value.inline.h b/be/src/runtime/decimal-value.inline.h index cb889e6..29d0159 100644 --- a/be/src/runtime/decimal-value.inline.h +++ b/be/src/runtime/decimal-value.inline.h @@ -674,21 +674,25 @@ inline std::string DecimalValue<T>::ToString(const ColumnType& type) const { template<typename T> inline std::string DecimalValue<T>::ToString(int precision, int scale) const { + T value; + // 'value_' may be unaligned. Use memcpy to avoid emitting instructions that assume + // alignment - see IMPALA-7473. + memcpy(&value, &value_, sizeof(T)); // Decimal values are sent to clients as strings so in the interest of // speed the string will be created without the using stringstream with the // whole/fractional_part(). int last_char_idx = precision + (scale > 0) // Add a space for decimal place + (scale == precision) // Add a space for leading 0 - + (value_ < 0); // Add a space for negative sign + + (value < 0); // Add a space for negative sign std::string str = std::string(last_char_idx, '0'); // Start filling in the values in reverse order by taking the last digit // of the value. Use a positive value and worry about the sign later. At this // point the last_char_idx points to the string terminator. - T remaining_value = value_; + T remaining_value = value; int first_digit_idx = 0; - if (value_ < 0) { - remaining_value = -value_; + if (value < 0) { + remaining_value = -value; first_digit_idx = 1; } if (scale > 0) { @@ -710,7 +714,7 @@ inline std::string DecimalValue<T>::ToString(int precision, int scale) const { } // For safety, enforce string length independent of remaining_value. } while (last_char_idx > first_digit_idx); - if (value_ < 0) str[0] = '-'; + if (value < 0) str[0] = '-'; return str; } http://git-wip-us.apache.org/repos/asf/impala/blob/9af2fdc4/fe/src/main/java/org/apache/impala/planner/PlanNode.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/PlanNode.java b/fe/src/main/java/org/apache/impala/planner/PlanNode.java index 208c49b..57d3bf4 100644 --- a/fe/src/main/java/org/apache/impala/planner/PlanNode.java +++ b/fe/src/main/java/org/apache/impala/planner/PlanNode.java @@ -341,11 +341,15 @@ abstract public class PlanNode extends TreeNode<PlanNode> { if (detailLevel.ordinal() >= TExplainLevel.EXTENDED.ordinal()) { expBuilder.append(detailPrefix); expBuilder.append("in pipelines: "); - List<String> pipelines = Lists.newArrayList(); - for (PipelineMembership pipe: pipelines_) { - pipelines.add(pipe.getExplainString()); + if (pipelines_ != null) { + List<String> pipelines = Lists.newArrayList(); + for (PipelineMembership pipe: pipelines_) { + pipelines.add(pipe.getExplainString()); + } + expBuilder.append(Joiner.on(", ").join(pipelines) + "\n"); + } else { + expBuilder.append("<not computed>"); } - expBuilder.append(Joiner.on(", ").join(pipelines) + "\n"); }
