[peco-engineer-bot]

Summary

Automated fix for #901 — [coverage] Conformance findings: PARAMQUERY-021.

I fixed DecimalParameter._cast_expr in src/databricks/sql/parameters/native.py. It now sends DECIMAL(precision, scale) instead of DECIMAL(scale, precision). It checks is not None rather than truthiness, so scale=0 is honoured. It also widens an under-declared scale up to the value's own scale (capped at 38), so no fractional digits are dropped. I corrected the swapped scale/precision docstring as well. The 4 new E2E tests now pass against the live warehouse on both Thrift and SEA, along with the new unit test. The full unit suite passes, and so do the existing decimal E2E tests in tests/e2e/test_parameterized_queries.py.

Root cause & plan

Root cause: In src/databricks/sql/parameters/native.py, DecimalParameter._cast_expr() builds the cast string as CAST_EXPR.format(self.scale, self.precision), i.e. "DECIMAL(scale,precision)". SQL wants DECIMAL(precision,scale). The constructor docstring also mixes up the two names: it calls scale the total digit count and precision the fractional digits. So a caller using the standard names, DecimalParameter(Decimal('123.45'), scale=2, precision=10), sends DECIMAL(2,10). Thrift rejects that with INVALID_PARAMETER_MARKER_VALUE.INVALID_DATA_TYPE / 22023, and the SEA/kernel path rejects it with "DECIMAL scale must be in 0..=precision". Two smaller problems: (1) the if self.scale and self.precision truthiness check sends scale=0 down the inference path; (2) nothing stops a declared scale smaller than the value's own scale. For example, declared DECIMAL(10,2) with 123.4567 casts down and loses digits, but the spec (facet c) says the value's scale must win. Facet (b), with no declared scale, already works: calculate_decimal_cast_string infers DECIMAL(5,2) for 123.45. No test is planned for it.
Files: src/databricks/sql/parameters/native.py, tests/e2e/test_parameterized_queries.py, tests/unit/test_parameters.py
Planned coverage:

• E2E in tests/e2e/test_parameterized_queries.py (default Thrift backend): cursor.execute('SELECT ? AS v', [DecimalParameter(Decimal('123.45'), scale=2, precision=10)]) succeeds and the result == Decimal('123.45'). Fails today with the server error INVALID_DATA_TYPE because DECIMAL(2,10) is sent. ((a) explicit precision/scale order — thrift)
• Same assertion with the connection opened with use_sea=True (backend-parametrized case). Fails today because the server/kernel rejects DECIMAL(2,10). ((a) explicit precision/scale order — SEA)
• E2E (thrift + sea params): 'SELECT ? AS v' with DecimalParameter(Decimal('123.4567'), scale=2, precision=10) returns Decimal('123.4567'), all four fractional digits kept, not 123.46 or 123. Fix: in _cast_expr, when precision/scale are given, use effective_scale = max(declared scale, value scale) and widen precision so the integer digits still fit (capped at 38). ((c) value scale authoritative over under-declared target scale)
• Unit test in tests/unit/test_parameters.py: DecimalParameter(Decimal('123.45'), scale=2, precision=10)._cast_expr() == 'DECIMAL(10,2)'. Add a scale=0 case too: Decimal('123'), scale=0, precision=10 -> 'DECIMAL(10,0)'. This is a supplement to the E2E tests, not a replacement. (unit: cast string ordering)

Files changed

• tests/e2e/test_parameterized_queries.py
• tests/unit/test_parameters.py
• src/databricks/sql/parameters/native.py

Test plan

• tests/e2e/test_parameterized_queries.py::TestParameterizedQueries::test_decimal_explicit_precision_scale_order[extra_params0] — fails (red) against the original code, passes (green) after the fix
• tests/e2e/test_parameterized_queries.py::TestParameterizedQueries::test_decimal_explicit_precision_scale_order[extra_params1] — fails (red) against the original code, passes (green) after the fix
• tests/e2e/test_parameterized_queries.py::TestParameterizedQueries::test_decimal_target_type_never_truncates_bound_scale[extra_params0] — fails (red) against the original code, passes (green) after the fix
• tests/e2e/test_parameterized_queries.py::TestParameterizedQueries::test_decimal_target_type_never_truncates_bound_scale[extra_params1] — fails (red) against the original code, passes (green) after the fix
• tests/unit/test_parameters.py::test_decimal_explicit_precision_scale_cast_string — fails (red) against the original code, passes (green) after the fix

🤖 Generated by engineer-bot (bug-fix flow) — review before merge.

[peco-review-bot]

Verdict: 1 High · 2 Medium · 1 Nit

Fixes the swapped DECIMAL(scale,precision) bug correctly, but there's one high concern: callers who followed the old (swapped) docstring, including examples/parameters.py, are now silently read the other way around and get a different type. The new widening logic also has no unit coverage. Nit: the docstring at tests/e2e/test_kernel_backend.py:690-698 still describes the explicit-arg bug as pre-existing and out of scope, and should be updated now that it's fixed.

Other findings

• ⚪ Nit — The except AttributeError fallback means a non-Decimal value (e.g. a str like "123.4567" or a float, which the constructor doesn't reject) skips widening without any signal and is sent with the under-declared scale. The value's digits would then be truncated, which is exactly what this PR sets out to prevent. Consider coercing with decimal.Decimal(str(self.value)) before calling as_tuple() so behaviour doesn't depend on the input type.

[peco-review-bot]

Verdict: 1 Medium · 2 Nit

The fix is correct: the cast now sends DECIMAL(precision, scale), scale=0 is handled, and an under-declared scale is widened. I found one medium issue and two low ones. The medium one is that the new scale > precision ValueError breaks callers who followed the old documented convention and had working code. The other two are about how the 38-digit cap behaves and a docstring claim that the cap contradicts.

Other findings

• ⚪ Nit — When the widened type goes over 38 digits, the cap cuts integer digits instead of scale. That can produce a type that can't hold the value, even when a valid one exists. Example: DecimalParameter(Decimal('1'*30 + '.5'), precision=12, scale=10) gives value_int_digits=30 and effective_scale=10. That's 40 digits, capped to DECIMAL(38,10), which leaves only 28 integer digits for a 30-digit value. The server will overflow (an error in ANSI mode, possibly a silent NULL otherwise). DECIMAL(31,1) would hold the value exactly, and that's what the inference path returns.

When capping, shrink the declared scale back toward value_scale before losing integer digits. For example:
effective_scale = max(value_scale, min(effective_scale, MAX_PRECISION - int_digits)), then cap. Add a unit case for it.

• ⚪ Nit — The new docstring says the value is "never rounded or truncated to a narrower declared type." That isn't true in two cases this PR handles itself:
• When the value needs more than 38 digits, the type is capped. The new unit test expects DECIMAL(38,38) for a 40-fractional-digit value, so digits are dropped.
• For non-Decimal values with no as_tuple, e.g. 1.2345 with scale=2, the declared type is used unchanged, so the value gets truncated.

Either soften the wording ("up to the 38-digit maximum, for Decimal values") or reject these inputs explicitly.

[peco-review-bot]

Verdict: 1 Medium · 2 Low

The fix itself looks right: DECIMAL(precision, scale) ordering, scale=0 handling, and the legacy swap with a FutureWarning all behave correctly, and the allocation math stays within 0..38. I have one medium concern: the change also widens integer digits, which the issue didn't ask for and which silently removes the server's overflow error. Two low findings cover range validation and non-Decimal inputs.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Looks good overall: the fix to the DECIMAL(precision, scale) order, the scale=0 handling, the legacy-swap warning and scale widening are correct and well covered by unit and E2E tests. One medium concern: widening the integer room goes beyond what the issue asked for. Nit: the debug log message hard-codes "38" instead of using MAX_PRECISION.

[peco-review-bot]

Verdict: 1 Medium · 2 Low

The core fix is correct: the cast is now DECIMAL(precision, scale), scale=0 is honoured, and the widening keeps all of the value's digits, with good unit and E2E coverage. One medium issue: two new lines exceed black's 88-char default, so the CI black --check src gate should fail. There are also two low concerns: the integer-room widening goes beyond what the issue asks for, and the CHANGELOG doesn't mention the new ValueError cases.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Mostly good: the precision/scale order is fixed and the tests are thorough. One medium issue: callers using the old convention, which already worked, now silently stop rounding because widening runs after the legacy swap. One low issue: error messages after the swap can name the wrong argument.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

The fix is mostly sound and well tested: DECIMAL(precision, scale) ordering, the is not None checks, and the widening logic all look right. There is one medium issue: legacy calls with precision=0 used to fall back to type inference, and now they are swapped and rounded to an integer scale. The duplicated validation blocks are also mostly unreachable.

[peco-review-bot]

Verdict: 2 Low

Looks good overall, with 2 low concerns. The DECIMAL(precision, scale) ordering fix, the is not None check, the legacy-swap handling and the widening logic all look right on the edge cases I traced (cap at 38, integer overflow, NaN, positive exponents). Unit and E2E coverage is thorough.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low concern. The precision/scale order fix, the legacy-swap handling with a FutureWarning, and the scale-widening logic are all correct, and the unit and E2E tests cover them well. The remaining issue is that existing calls passing scale=0 used to get an inferred type and now bind the declared type, with no warning and no CHANGELOG entry.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity edge case. The fix correctly binds DECIMAL(precision, scale), honours scale=0, and widens an under-declared scale. The legacy-swap, precision=0 and scale=0 paths keep backward compatibility and have thorough unit tests. Nit: the CHANGELOG entry is one very long bullet; splitting it into sub-bullets (ordering fix, widening, new validation, deprecations) would make it easier to read.

[peco-review-bot]

🔵 Low — The precision == 0 fallback runs before the range checks and sets scale/precision to None. As a result, scale is never validated on this path. DecimalParameter(v, scale=-1, precision=0) and DecimalParameter(v, scale=500, precision=0) only emit the FutureWarning and fall back to inference. The equivalent call with a nonzero precision, such as scale=-1, precision=10, raises ValueError("scale must be >= 0").

The CHANGELOG says a negative scale now raises, but that is not true when precision=0. Earlier releases accepted these inputs too, so this is not a regression. Still, a clearly invalid scale is probably better reported here than hidden behind a migration warning. Suggested fix: run the scale < 0 / scale > MAX_PRECISION checks before the precision=0 fallback, or note this exception in the CHANGELOG.

[peco-engineer-bot]

Add a test for the precision=0 invalid-scale case.

Pushed 1bda507.

[peco-review-bot]

Thanks for picking this up. I can't see the fix at head, though. The precision == 0 branch still runs before the range checks and sets both fields to None, so DecimalParameter(v, scale=-1, precision=0) and scale=500, precision=0 still only warn and fall back to inference. The only precision=0 test in tests/unit/test_parameters.py is test_decimal_precision_zero_falls_back_to_inferred_type_with_warning, and it uses only valid scales (0, 5, 10), so it doesn't cover the invalid-scale case. The CHANGELOG still says "a negative scale" raises, without mentioning that precision=0 is an exception. To close this, either move the scale < 0 / scale > MAX_PRECISION checks above the precision=0 fallback and add a test asserting ValueError for those inputs, or note the exception in the CHANGELOG. I'm leaving this open for a human to look at.

[peco-engineer-bot]

⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.

The fix is already in the current branch head; I made no edits and the working tree is clean. In DecimalParameter.__init__ (src/databricks/sql/parameters/native.py), the scale < 0 and scale > MAX_PRECISION checks now run before the precision == 0 fallback, so scale=-1, precision=0 and scale=500, precision=0 raise ValueError instead of only warning. The new test test_decimal_precision_zero_with_invalid_scale_raises in tests/unit/test_parameters.py covers both inputs and turns FutureWarning into an error, so the migration warning can't hide a missing raise. tests/unit/test_parameters.py passes (104 tests, run with -m "not realkernel"). The CHANGELOG line about a negative scale raising is now true on every path, so it needs no exception note. The review-bot probably looked at an older ref. A human should confirm the fix at head and resolve the thread.

[peco-review-bot]

Verdict: 1 Low

This looks good, with one low concern. The precision/scale ordering fix, the is not None handling, the input validation and the backward-compat warnings are carefully done and well covered by tests. The remaining gap is that the legacy-convention case where scale == precision now gets widened silently instead of keeping the old bound type. Nit: test_decimal_widened_type_is_logged and test_decimal_widened_scale_only_is_not_warned build the same parameter and could be merged.

[peco-review-bot]

✅ No issues identified by the review bot.