fix: coerce operand types in Interval mul/div/intersect/union/contains#22027
Open
adriangb wants to merge 1 commit intoapache:mainfrom
Open
fix: coerce operand types in Interval mul/div/intersect/union/contains#22027adriangb wants to merge 1 commit intoapache:mainfrom
adriangb wants to merge 1 commit intoapache:mainfrom
Conversation
…`/`contains` Previously these methods asserted that both intervals shared an identical data type, causing internal errors during interval propagation for queries like `numeric / count(*)` where the operands end up as different `Decimal128` precisions/scales (e.g. `Decimal128(38, 10) / Decimal128(20, 0)`). `Interval::add` and `Interval::sub` already used `BinaryTypeCoercer` to find a common arithmetic type. This change brings `mul`/`div` in line, and similarly relaxes `intersect`/`union`/`contains` to coerce via `comparison_coercion`, since CP-solver propagation feeds the result of an arithmetic op into `intersect` with a child interval whose type may differ. Adds a unit test in `interval_arithmetic.rs` for mismatched `Decimal128` mul/div, and an end-to-end `decimal.slt` regression covering the `numeric / bigint` shape from the failing customer query. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
86c7d43 to
097259d
Compare
Contributor
Author
|
@pepijnve @neilconway any interest in reviewing this fix? |
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
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.
Which issue does this PR close?
Rationale for this change
Interval::mul,Interval::div,Interval::intersect,Interval::union, andInterval::containsall asserted that both operands had identical data types. This causes internal errors during interval propagation for ordinary SQL queries that mixDecimal128precisions/scales — for example dividingnumeric(which DataFusion maps toDecimal128(38, 10)) by aBIGINTcolumn orcount(*)(coerced toDecimal128(20, 0)):Interval::addandInterval::subalready usedBinaryTypeCoercerto find a common arithmetic type; this PR bringsmulanddivin line. Oncemul/divcoerce, the result of an arithmetic op fed intointersect(e.g. by the CP solver incp_solver.rs) may have a different type than the child interval, sointersect/union/containsare also relaxed to coerce viacomparison_coercion(matching the existing pattern inInterval::contains_value).Reproducer (
datafusion-cli, before this PR)fails with the internal-error message above. After this PR the query returns rows.
What changes are included in this PR?
Interval::mulandInterval::divwith a newcoerce_operandshelper that usesBinaryTypeCoercer::get_result_typeand casts both intervals to the common type when they differ.Interval::intersect,Interval::union, andInterval::containswith a newcoerce_for_comparisonhelper that usescomparison_coercionand casts both intervals to the common type when they differ.The same-type fast path is preserved (no allocation/cast when both intervals already share a type), so this should be a no-op for existing call sites.
Are these changes tested?
Yes:
test_mul_div_mismatched_operand_typesininterval_arithmetic.rsexercisingDecimal128(38, 10)/Decimal128(20, 0)andDecimal128 × Int64for bothmulanddiv.decimal.sltcovering thenumeric / bigintshape from the reproducer above through the optimizer's interval-propagation path. Without this fix the SLT query fails with an internal error; with it, it returns rows.datafusion-expr-common,datafusion-physical-expr,datafusion-physical-plan, and fullsqllogictestssuites all pass.Are there any user-facing changes?
Queries that previously hit
Internal error: Intervals must have the same data type ...(or the equivalent for intersect/union/contains) during interval/stats propagation will now succeed. No public API changes — these methods kept their signatures; the documented preconditions are relaxed.🤖 Generated with Claude Code