SEPURI-SAI-KRISHNA commented on PR #12899: URL: https://github.com/apache/seatunnel/pull/12899#issuecomment-6416309477
Thanks @DanielLeens. Issue 1 is addressed in `275efb97b`, and chasing it turned up something your note did not cover, so the fix is a bit larger than the one sentence you asked for. **You were right.** I verified it instead of taking it on reading, with `bigint_col` null, `decimal_col` = 18446744073709551616 and `double_col` = 1e30: | expression | result | | --- | --- | | `CAST(decimal_col AS BIGINT)` | throws, no such cast | | `CAST(double_col AS BIGINT)` | throws, no such cast | | `COALESCE(bigint_col, decimal_col)` | throws, the new range check | | `COALESCE(decimal_col, bigint_col)` | `Decimal(38, 0)`, value preserved exactly | | `COALESCE(double_col, bigint_col)` | `DOUBLE`, `1.0E30` | So a reader with a `DECIMAL` column following the line I added yesterday would go looking for a cast that does not exist. Reordering is the only remedy, exactly as you said. **The part worth flagging: reordering is not equivalent.** Before documenting your advice I checked what it does when both arguments are non-null, with `bigint_col` = 42 and `decimal_col` = 777: ```text COALESCE(bigint_col, decimal_col) -> 42 COALESCE(decimal_col, bigint_col) -> 777 ``` The result type changes as intended, but so does which argument wins. The two forms only agree when the first argument is the one that can be null, which is the failing case but not the general one. Advising a bare reorder would have let someone trade an exception for silently different data, which is the same category of problem this PR exists to remove. So the note now says both things: for an integral source write the cast, for `DECIMAL` or `DOUBLE` put it first since no cast exists, and reordering is not a drop-in change because it also changes which argument wins when both are non-null. That is in all four files, the EN and ZH `COALESCE` and `IFNULL` sections and both Migration Guides. `275efb97b` touches 0 production files, docs only, 4 files `+21/-3`. **On your Issue 2**: the PR-signal workflow has fired on this head, so a fresh `Build` is on its way. I will report what it says rather than assume it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
