fix(tests): correct decimal sum and nullif results#1048
Open
bvolpato wants to merge 2 commits into
Open
Conversation
bvolpato-dd
force-pushed
the
fix/strict-type-parameter-checker
branch
from
086041e to
85d16cd
Compare
bvolpato
requested review from
EpsilonPrime,
benbellick,
cpcloud,
jacques-n,
nielspardon,
vbarua,
westonpace and
yongchul
as code owners
benbellick
reviewed
Apr 15, 2026
Member
Author
|
[review-prs-verification] Files reviewed:
Changes:
Tests/checks:
|
|
This PR has been automatically marked as stale because it has not had |
bvolpato
force-pushed
the
fix/strict-type-parameter-checker
branch
from
85d16cd to
8cfd889
Compare
…sults The coverage checker's `is_same_type` compared only the base type name and stripped all parameters, so decimal precision/scale, varchar length, and list/map element types were never actually validated against the extension YAML return formulas. Two wrong test cases had been sitting in-tree because of this: - `sum((2.5, 0, 5.0, -2.5, -7.5)::dec<2, 1>) = -2.5::dec<38, 2>` in `tests/cases/arithmetic_decimal/sum_decimal.test`. `sum` returns `DECIMAL?<38,S>`, so with input scale 1 the output must be `dec?<38, 1>`, not `dec?<38, 2>`. - `nullif(null::dec?<38, 0>, null::dec?<38, 0>) = null::bool?` in `tests/cases/comparison/nullif.test`. `nullif` is `any1, any1 -> any1?`, so with both args `dec<38, 0>` the result must be `dec?<38, 0>`, not `bool?`. Looks like a copy-paste from the bool cases above. Both wrong results date back to the original BFT port (substrait-io#738). They are not undoing prior work: substrait-io#913 only touched the `basic` i8/i16 block at the top of `nullif.test`, and substrait-io#989 rewrote both lines only to add `?` nullability markers, preserving the underlying wrong `38,2` and wrong `bool` base type. This change keeps every `?` marker from substrait-io#989 and only fixes the parameter substrait-io#989 wasn't looking at. To prevent regressions, this adds `tests/coverage/type_checker.py` — a symbolic unifier plus evaluator for the YAML return-formula mini-language (assignments, `min`/`max`, `cond ? a : b` ternary). The new module: - parses type strings like `decimal<P1,S1>`, `list<any2>`, `STRUCT<...>`, `func<any1 -> boolean?>` into tagged tuples; - unifies an impl-declared type against a concrete test type, binding variables like `P1`, `S1`, and polymorphic `any1`/`any2`; - evaluates multi-line return formulas (add/sub/mul/div/mod, min/max, sum, `any1?`, etc.) with the bindings and compares structurally to the test's declared result type. `FunctionOverload`/`FunctionVariant` now carry the raw YAML arg types and return formula alongside the existing short-form fingerprint. `FunctionRegistry.get_function` runs the strict check after the legacy loose match, so the wider test suite's base-type fallback still applies when the caller hasn't supplied full parameterized types. When the formula cannot be evaluated (unbound variable, unusual syntax), the strict check falls back to success, preserving compatibility with the current extensions and leaving room to tighten further. `test_type_checker.py` covers parsing, unification, formula evaluation for add/divide, the two specific bugs above, and the tolerant behavior for tests that omit optional decimal parameters (e.g. `power(dec, dec<38,0>)`).
Keep the two verified testcase corrections and defer strict parameter validation until it can reuse the generated ANTLR grammar.
bvolpato
force-pushed
the
fix/strict-type-parameter-checker
branch
from
7881c4a to
2e77365
Compare
bvolpato
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Correct two inconsistent expected result types in the function test corpus:
sumpreserves input scaleS, sodec<2,1>returnsdec?<38,1>nullif(any1, any1) -> any1?, so decimal arguments return nullable decimal rather than nullable booleanThe earlier strict type-checker implementation has been removed. A future checker can reuse the generated ANTLR type grammar without adding a second parser.
Testing
pixi run lintpixi run test(137 passed)This change is