Skip to content

Commit

Permalink
fix(datatypes): ensure that decimals can be upcast when source precis…
Browse files Browse the repository at this point in the history
…ion, scale are lte to their target fields (#10470)

Looks like we had some flipped logic in the `castable` branch for
decimals that did not allow upcasting. Fixes #10467.
  • Loading branch information
cpcloud authored Nov 11, 2024
1 parent caf3632 commit 5ce906a
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 6 deletions.
33 changes: 33 additions & 0 deletions ibis/backends/tests/test_generic.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
ImpalaHiveServer2Error,
MySQLProgrammingError,
OracleDatabaseError,
PolarsInvalidOperationError,
PolarsSchemaError,
PsycoPg2InternalError,
PsycoPg2SyntaxError,
Expand Down Expand Up @@ -2413,3 +2414,35 @@ def test_named_literal(con, backend):
result = con.to_pandas(expr)
expected = pd.DataFrame({"one": [1]})
backend.assert_frame_equal(result, expected)


@pytest.mark.notyet(
["polars"],
raises=PolarsInvalidOperationError,
reason="n_unique isn't supported on decimal columns",
)
@pytest.mark.notyet(
["clickhouse"],
raises=ClickHouseDatabaseError,
reason="doesn't allow casting Float64 to Decimal(38, 2)",
)
@pytest.mark.notimpl(
["oracle"], raises=OracleDatabaseError, reason="incorrect code generated"
)
@pytest.mark.notimpl(
["datafusion", "flink", "impala", "mysql", "mssql", "sqlite", "trino"],
raises=com.OperationNotDefinedError,
reason="quantile not implemented",
)
@pytest.mark.notimpl(
["druid"],
raises=com.OperationNotDefinedError,
reason="standard deviation not implemented",
)
def test_table_describe_with_multiple_decimal_columns(con):
t = ibis.memtable({"a": [1, 2, 3], "b": [4, 5, 6]}).cast(
{"a": "decimal(21, 2)", "b": "decimal(20, 2)"}
)
expr = t.describe()
result = con.to_pyarrow(expr)
assert len(result) == 2
10 changes: 5 additions & 5 deletions ibis/expr/datatypes/cast.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,17 @@ def castable(source: dt.DataType, target: dt.DataType, value: Any = None) -> boo
return source.is_integer()
elif target.is_decimal():
if source.is_decimal():
downcast_precision = (
upcast_precision = (
source.precision is not None
and target.precision is not None
and source.precision < target.precision
and source.precision <= target.precision
)
downcast_scale = (
upcast_scale = (
source.scale is not None
and target.scale is not None
and source.scale < target.scale
and source.scale <= target.scale
)
return not (downcast_precision or downcast_scale)
return upcast_precision and upcast_scale
else:
return source.is_numeric()
elif target.is_string():
Expand Down
23 changes: 23 additions & 0 deletions ibis/expr/datatypes/tests/test_cast.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,26 @@ def test_struct_different_fields():
# Missing fields entirely from each other
assert not x.castable(y)
assert not y.castable(x)


@pytest.mark.parametrize(
("source", "target", "expected"),
[
# Fixed precision
((12, 2), (12, 3), True),
((12, 3), (12, 2), False),
# Fixed scale
((12, 2), (13, 2), True),
((13, 2), (12, 2), False),
# Equal
((12, 2), (12, 2), True),
# Not equal
((12, 2), (13, 3), True),
((13, 2), (12, 3), False),
((13, 2), (12, 1), False),
],
)
def test_castable_decimal_to_decimal(source, target, expected):
left = dt.Decimal(*source)
right = dt.Decimal(*target)
assert left.castable(right) is expected
7 changes: 6 additions & 1 deletion ibis/expr/types/relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -2981,7 +2981,12 @@ def describe(
)
aggs.append(agg)

t = ibis.union(*aggs)
names = aggs[0].schema().names
new_schema = {
name: dt.highest_precedence(types)
for name, *types in zip(names, *(agg.schema().types for agg in aggs))
}
t = ibis.union(*(agg.cast(new_schema) for agg in aggs))

# TODO(jiting): Need a better way to remove columns with all NULL
if string_col and not numeric_col:
Expand Down

0 comments on commit 5ce906a

Please sign in to comment.