Skip to content

Commit 2b3a85a

Browse files
ai: apply changes for #1006 (2 review threads)
Addresses: - #4418552981 at src/databricks/sql/parameters/native.py:674 - #4418552988 at src/databricks/sql/parameters/native.py:677 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
1 parent 6177ab4 commit 2b3a85a

2 files changed

Lines changed: 65 additions & 6 deletions

File tree

‎src/databricks/sql/parameters/native.py‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -640,13 +640,36 @@ def __init__(
640640
self.name = name
641641
self.scale = scale
642642
self.precision = precision
643+
self._legacy_swapped = False
643644

644645
if not self.valid_scale_and_precision():
645646
raise ValueError(
646647
"DecimalParameter requires both or none of scale and precision to be set, "
647648
"as integers"
648649
)
649650

651+
if self.scale is not None and self.precision is not None:
652+
# Range-check the raw arguments *before* the legacy swap below, so a call
653+
# that is simply invalid (rather than written for the legacy convention)
654+
# gets an error naming the argument the caller actually passed, and no
655+
# misleading migration warning.
656+
if self.scale < 0:
657+
raise ValueError(
658+
"DecimalParameter scale must be >= 0, got {}".format(self.scale)
659+
)
660+
if self.precision < 0 or self.precision > self.MAX_PRECISION:
661+
raise ValueError(
662+
"DecimalParameter precision must be between 1 and {}, got {}".format(
663+
self.MAX_PRECISION, self.precision
664+
)
665+
)
666+
if self.scale > self.MAX_PRECISION:
667+
raise ValueError(
668+
"DecimalParameter scale must be between 0 and {}, got {}".format(
669+
self.MAX_PRECISION, self.scale
670+
)
671+
)
672+
650673
if (
651674
self.scale is not None
652675
and self.precision is not None
@@ -663,7 +686,10 @@ def __init__(
663686
"`precision` is the total number of digits and `scale` is the number of "
664687
"digits after the decimal point, as in SQL DECIMAL(precision, scale). "
665688
"Swap your arguments; this will raise ValueError in a future "
666-
"release.".format(
689+
"release. Legacy-convention calls keep the previous behaviour (the "
690+
"server rounds the value to the declared scale); with correctly "
691+
"ordered arguments the bound scale is widened to fit the value's "
692+
"fractional digits instead.".format(
667693
self.scale, self.precision, self.scale, self.precision
668694
),
669695
# FutureWarning (not DeprecationWarning) so end users see it by
@@ -672,6 +698,9 @@ def __init__(
672698
stacklevel=2,
673699
)
674700
self.scale, self.precision = self.precision, self.scale
701+
# Preserve the old bound type exactly (no widening) for these callers,
702+
# so the only change they see is the warning.
703+
self._legacy_swapped = True
675704

676705
if self.scale is not None and self.precision is not None:
677706
if self.scale < 0:
@@ -700,6 +729,10 @@ def _is_int(x):
700729

701730
def _cast_expr(self):
702731
if self.scale is not None and self.precision is not None:
732+
if self._legacy_swapped:
733+
# Legacy-convention call: bind the declared type unchanged, as
734+
# earlier releases did, so existing rounding is preserved.
735+
return self.CAST_EXPR.format(self.precision, self.scale)
703736
precision, scale = self._effective_precision_and_scale(
704737
self.precision, self.scale
705738
)

‎tests/unit/test_parameters.py‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import datetime
2+
import warnings
23
from decimal import Decimal
34
from enum import Enum
45
from typing import Type
@@ -190,6 +191,22 @@ def test_decimal_scale_greater_than_precision_legacy_swapped():
190191
assert p._cast_expr() == "DECIMAL(10,2)"
191192

192193

194+
def test_decimal_legacy_swapped_is_not_widened():
195+
"""Legacy-convention calls keep the old bound type (and so the old rounding):
196+
DecimalParameter(Decimal("12.3456"), scale=4, precision=2) still binds DECIMAL(4,2)
197+
rather than being widened to DECIMAL(6,4)."""
198+
with pytest.warns(FutureWarning, match="previous behaviour"):
199+
p = DecimalParameter(Decimal("12.3456"), scale=4, precision=2)
200+
assert p._cast_expr() == "DECIMAL(4,2)"
201+
assert p.as_tspark_param(named=False).type == "DECIMAL(4,2)"
202+
203+
204+
def test_decimal_correct_order_with_more_fractional_digits_is_widened():
205+
"""Counterpart of the legacy test: correctly ordered arguments do widen."""
206+
p = DecimalParameter(Decimal("12.3456"), scale=2, precision=4)
207+
assert p._cast_expr() == "DECIMAL(6,4)"
208+
209+
193210
@pytest.mark.parametrize(
194211
"scale,precision,match",
195212
(
@@ -213,11 +230,20 @@ def test_decimal_bool_scale_or_precision_raises(scale, precision):
213230
DecimalParameter(Decimal("1.5"), scale=scale, precision=precision)
214231

215232

216-
def test_decimal_legacy_swapped_out_of_range_precision_raises():
217-
"""The range check applies after the legacy swap (scale=50 becomes precision=50)."""
218-
with pytest.warns(FutureWarning, match="legacy"):
219-
with pytest.raises(ValueError, match="precision must be between 1 and 38"):
220-
DecimalParameter(Decimal("1.5"), scale=50, precision=2)
233+
@pytest.mark.parametrize(
234+
"scale,precision,match",
235+
(
236+
(50, 2, "scale must be between 0 and 38, got 50"),
237+
(5, -1, "precision must be between 1 and 38, got -1"),
238+
),
239+
)
240+
def test_decimal_out_of_range_raw_args_raise_before_legacy_swap(scale, precision, match):
241+
"""Range checks run on the raw arguments before the legacy swap, so the error
242+
names the argument the caller actually passed and no FutureWarning is emitted."""
243+
with warnings.catch_warnings():
244+
warnings.simplefilter("error", FutureWarning)
245+
with pytest.raises(ValueError, match=match):
246+
DecimalParameter(Decimal("1.5"), scale=scale, precision=precision)
221247

222248

223249
class Primitive(Enum):

0 commit comments

Comments
 (0)