[client] Improve message when setting a decimal on a non-decimal column

This patch improves the error messaging when setting a decimal on a non-decimal column. Changes the order of validation operations to first check the column type is valid before checking the value is in range. Change-Id: I331028c3ce88e54eef0a091c0cc98b39293fb3c1 Reviewed-on: http://gerrit.cloudera.org:8080/12433 Tested-by: Grant Henke <[email protected]> Reviewed-by: Andrew Wong <[email protected]>
WentaoRen · Feb 11, 2019 · 4f3baf7 · 4f3baf7
1 parent 09d53a5
commit 4f3baf7
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 6 deletions.
diff --git a/src/kudu/client/scan_batch.cc b/src/kudu/client/scan_batch.cc
@@ -341,7 +341,7 @@ Status KuduScanBatch::RowPtr::GetUnscaledDecimal(int col_idx, int128_t* val) con
       return Status::OK();
     default:
       return Status::InvalidArgument(
-          Substitute("invalid type $0 provided for column '$1' (expected DECIMAL)",
+          Substitute("invalid type $0 provided for column '$1' (expected decimal)",
                      col.type_info()->name(), col.name()));
   }
 }

diff --git a/src/kudu/common/partial_row-test.cc b/src/kudu/common/partial_row-test.cc
@@ -232,6 +232,12 @@ TEST_F(PartialRowTest, UnitTest) {
   EXPECT_EQ("Invalid argument: value -10000.00 out of range for decimal column 'decimal_val'",
             s.ToString());
 
+  // Set a decimal value on a non decimal column.
+  s = row.SetUnscaledDecimal("string_val", 123456);
+  EXPECT_EQ("Invalid argument: invalid type string provided for column "
+            "'string_val' (expected decimal)",
+            s.ToString());
+
   // Even though the storage is actually the same at the moment, we shouldn't be
   // able to set string columns with SetBinary and vice versa.
   EXPECT_FALSE(row.SetBinaryCopy("string_val", "oops").ok());

diff --git a/src/kudu/common/partial_row.cc b/src/kudu/common/partial_row.cc
@@ -301,27 +301,34 @@ Status KuduPartialRow::SetFloat(int col_idx, float val) {
 Status KuduPartialRow::SetDouble(int col_idx, double val) {
   return Set<TypeTraits<DOUBLE> >(col_idx, val);
 }
-Status KuduPartialRow::SetUnscaledDecimal(int col_idx, int128_t val) {
-  const ColumnSchema& col = schema_->column(col_idx);
-  const DataType col_type = col.type_info()->type();
 
+Status CheckDecimalValueInRange(ColumnSchema col, int128_t val) {
   int128_t max_val = MaxUnscaledDecimal(col.type_attributes().precision);
   int128_t min_val = -max_val;
   if (val < min_val || val > max_val) {
     return Status::InvalidArgument(
         Substitute("value $0 out of range for decimal column '$1'",
                    DecimalToString(val, col.type_attributes().scale), col.name()));
   }
+  return Status::OK();
+}
+
+Status KuduPartialRow::SetUnscaledDecimal(int col_idx, int128_t val) {
+  const ColumnSchema& col = schema_->column(col_idx);
+  const DataType col_type = col.type_info()->type();
   switch (col_type) {
     case DECIMAL32:
+      RETURN_NOT_OK(CheckDecimalValueInRange(col, val))
       return Set<TypeTraits<DECIMAL32> >(col_idx, static_cast<int32_t>(val));
     case DECIMAL64:
+      RETURN_NOT_OK(CheckDecimalValueInRange(col, val))
       return Set<TypeTraits<DECIMAL64> >(col_idx, static_cast<int64_t>(val));
     case DECIMAL128:
+      RETURN_NOT_OK(CheckDecimalValueInRange(col, val))
       return Set<TypeTraits<DECIMAL128> >(col_idx, static_cast<int128_t>(val));
     default:
       return Status::InvalidArgument(
-          Substitute("invalid type $0 provided for column '$1' (expected DECIMAL)",
+          Substitute("invalid type $0 provided for column '$1' (expected decimal)",
                      col.type_info()->name(), col.name()));
   }
 }
@@ -700,7 +707,7 @@ Status KuduPartialRow::GetUnscaledDecimal(int col_idx, int128_t *val) const {
       return Status::OK();
     default:
       return Status::InvalidArgument(
-          Substitute("invalid type $0 provided for column '$1' (expected DECIMAL)",
+          Substitute("invalid type $0 provided for column '$1' (expected decimal)",
                      col.type_info()->name(), col.name()));
   }
 }