Repository navigation
[CALCITE-7880] Unqualified common column of JOIN USING has the left column's type, so arithmetic on it can overflow - #5361
Conversation
| /** Regression test for | ||
| * <a href="https://issues.apache.org/jira/browse/CALCITE-7880">[CALCITE-7880] | ||
| * Arithmetic on an unqualified USING column can overflow its common type</a>. */ | ||
| @ParameterizedTest |
There was a problem hiding this comment.
These tests are hard to read for a human, but let's keep them since they are parameterized.
But can you add a comment explaining how you have validated that their results are correct?
Can you also add at least one quidem test containing the example of the issue?
There was a problem hiding this comment.
Added in 9650c0bdb. The Java comments now derive the expected rows from the matched/unmatched keys and explain the type controls. sql/join.iq contains the exact issue query with result and type assertions, preceded by a qualified f.k + 1 BIGINT arithmetic control.
Against the original validator, the qualified control passed and only the unqualified query differed: INTEGER/-2147483648 instead of BIGINT/2147483648. With the fix, all 47 core Quidem files pass. The complete default gradlew build also passed: 22,263 passing test records, zero failures/errors, with existing skips retained.
The two upstream workflows currently require repository approval. Could you approve this revision for CI?
| .ok(); | ||
| } | ||
|
|
||
| /** Tests that a common column has the same widened type with and without |
There was a problem hiding this comment.
Here you need to use the same standard format for the javadoc describing the issue.
There was a problem hiding this comment.
Added the standard CALCITE-7880 issue Javadoc to this validator regression in 9650c0bdb.
|
|
||
| @ParameterizedTest | ||
| @MethodSource("joinUsingMixedNumericNulls") | ||
| void testJoinUsingMixedNumericCommonColumnWithNulls(String join, String leftType, |
There was a problem hiding this comment.
This one needs the same comment pointing to the JIRA issue
There was a problem hiding this comment.
Added the standard CALCITE-7880 issue Javadoc here in 9650c0bdb, along with the derivation of the LEFT/RIGHT expected rows.
|
please squash the commits for merging |
mihaibudiu
added
the
LGTM-will-merge-soon
…olumn's type, so arithmetic on it can overflow Cast the selected common-column reference to the comparison common type before using it in an expression. Keep the chosen-side optimization for INNER, LEFT and RIGHT joins, and preserve FULL JOIN handling. Add JDBC and validator coverage, the exact issue query and its qualified BIGINT control in Quidem, and comments deriving the expected rows and types. Generated-by: OpenAI Codex
9650c0b to
529cc0d
Compare
|
Squashed into 529cc0d. The code tree is identical to the reviewed |
529cc0d to
580d7af
Compare
|
Fixed the commit-message line endings in 580d7af. |
|
Jira Link
CALCITE-7880
Changes Proposed
When an INTEGER key joins a BIGINT key with USING, the unqualified common column can retain the selected input's narrower type. For example, adding 1 to the common key 2147483647 returns -2147483648, while SELECT * exposes the key as BIGINT.
Cast the selected common-column reference to the comparison common type, using the same type-selection rule as star expansion. INNER/LEFT still select the left input and RIGHT still selects the right input, retaining CALCITE-2659's removal of redundant COALESCE. FULL JOIN and explicitly qualified input references retain their existing handling. The change preserves the selected expression's existing nullability.
The regression tests execute INTEGER/BIGINT inputs in both orders across INNER, LEFT, RIGHT, FULL and NATURAL joins, assert JDBC result types and qualified-column controls, and check nullable/unmatched rows for LEFT/RIGHT joins. Validator coverage includes direct common-column references with identifier expansion disabled and arithmetic with expansion enabled.
Validation on Windows with JDK 17 for tests and the project-default JDK 25 compilation toolchain:
gradlew.bat --no-daemon --no-parallel --max-workers=4 buildpassed, including formatting, Checkstyle, bytecode/API checks and the default test suites. JUnit XML reports 22,263 passed, zero failures/errors and 751 skipped entries; extra external-integration and slow-test profiles were not enabled.The focused run passed 24 tests (19 new cases plus 5 existing tests). On unmodified main, the new 19-case matrix had 9 failures and 10 passing controls.
Added a readable Quidem regression with the exact issue SQL and a qualified BIGINT control. Against the original validator, only the unqualified query failed; with this revision, all 47 core Quidem files pass.
AI assistance: implementation, test development and local validation used OpenAI Codex.