Skip to content

ARROW-4563: [Python] Validate decimal128() precision input #3647

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

ARROW-4563: [Python] Validate decimal128() precision input #3647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions cpp/src/arrow/type.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,15 @@ int StructType::GetChildIndex(const std::string& name) const {
return GetFieldIndex(name);
}

// ----------------------------------------------------------------------
// Decimal128 type

Decimal128Type::Decimal128Type(int32_t precision, int32_t scale)
: DecimalType(16, precision, scale) {
DCHECK_GE(precision, 1);
DCHECK_LE(precision, 38);
Copy link
Contributor

@pravindra pravindra Feb 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, check for scale >= 0 && scale <= precision ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. Why can't scale be arbitrary? It's simply an exponent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From this

Precision is the number of digits in a number. Scale is the number of digits to the right of the decimal point in a number. For example, the number 123.45 has a precision of 5 and a scale of 2.

The number of digits after the decimal must be >=0 and must be <= the digits in the number. Am I missing something ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is Microsoft-specific. There is no a priori reason why scale should be limited. Apparently Oracle allows scales between -128 and 127 (?):
https://docs.oracle.com/cd/A81042_01/DOC/server.816/a76965/c10datyp.htm#743

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the 0 to 38 thing is pretty ubiquitous in the database landscape

MapD seems to be capped at 19 digits of precision instead of 38, presumably to fit in 64 bits https://www.omnisci.com/docs/latest/5_datatypes.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pitrou - thanks, I didn't realize that oracle allows scale to be more than precision, and -ve also. The decimal functions in gandiva don't handle this.

However, I checked the links from @wesm - spark-sql, impala and presto (and of course, sql-server) all require scale to be <= precision.

}

// ----------------------------------------------------------------------
// DictionaryType

Expand Down
3 changes: 1 addition & 2 deletions cpp/src/arrow/type.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,8 +550,7 @@ class ARROW_EXPORT Decimal128Type : public DecimalType {
public:
static constexpr Type::type type_id = Type::DECIMAL;

explicit Decimal128Type(int32_t precision, int32_t scale)
: DecimalType(16, precision, scale) {}
explicit Decimal128Type(int32_t precision, int32_t scale);

Status Accept(TypeVisitor* visitor) const override;
std::string ToString() const override;
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/gandiva/expression_registry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ void ExpressionRegistry::AddArrowTypesToVector(arrow::Type::type& type,
vector.push_back(arrow::null());
break;
case arrow::Type::type::DECIMAL:
vector.push_back(arrow::decimal(0, 0));
vector.push_back(arrow::decimal(38, 0));
break;
case arrow::Type::type::FIXED_SIZE_BINARY:
case arrow::Type::type::MAP:
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/gandiva/function_registry_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ inline DataTypePtr time32() { return arrow::time32(arrow::TimeUnit::MILLI); }
inline DataTypePtr time64() { return arrow::time64(arrow::TimeUnit::MICRO); }

inline DataTypePtr timestamp() { return arrow::timestamp(arrow::TimeUnit::MILLI); }
inline DataTypePtr decimal128() { return arrow::decimal(0, 0); }
inline DataTypePtr decimal128() { return arrow::decimal(38, 0); }

struct KeyHash {
std::size_t operator()(const FunctionSignature* k) const { return k->Hash(); }
Expand Down
8 changes: 8 additions & 0 deletions python/pyarrow/tests/test_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,14 @@ def test_decimal_properties():
assert ty.scale == 4


def test_decimal_overflow():
pa.decimal128(1, 0)
pa.decimal128(38, 0)
for i in (0, -1, 39):
with pytest.raises(ValueError):
pa.decimal128(39, 0)


def test_type_equality_operators():
many_types = get_many_types()
non_pyarrow = ('foo', 16, {'s', 'e', 't'})
Expand Down
2 changes: 2 additions & 0 deletions python/pyarrow/types.pxi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1321,6 +1321,8 @@ cpdef DataType decimal128(int precision, int scale=0):
decimal_type : Decimal128Type
"""
cdef shared_ptr[CDataType] decimal_type
if precision < 1 or precision > 38:
raise ValueError("precision should be between 1 and 38")
decimal_type.reset(new CDecimal128Type(precision, scale))
return pyarrow_wrap_data_type(decimal_type)

Expand Down