Use FLOAT8_FITS_IN_INT64 in dsinterval_mul#1211
Use FLOAT8_FITS_IN_INT64 in dsinterval_mul#1211c2main wants to merge 4 commits intoIvorySQL:masterfrom
Conversation
dsinterval_mul() compared a rounded double directly to PG_INT64_MAX, which triggers Clang because that integer constant is not exactly representable as double near 2^63. Warning excerpt: ``` src/datatype/dsinterval.c:2336:22: warning: implicit conversion from long to double changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-const-int-float-conversion] ``` Follow the same integer-fit pattern used in timestamp.c by checking isnan() and FLOAT8_FITS_IN_INT64() after rint(). This avoids custom bound constants and keeps overflow checks consistent with core code.
📝 WalkthroughWalkthroughReplaced direct integer-range checks in dsinterval multiplication/division with NaN checks and FLOAT8_FITS_IN_INT32/INT64 guards; added tests for overflow and NaN cases; applied formatting/comment tweaks in yminterval including commenting out the year+month aggregation. No exported signatures changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contrib/ivorysql_ora/src/datatype/dsinterval.c (1)
2414-2419: Consider adding the same overflow check for consistency withdsinterval_mul.The
dsinterval_divfunction has similar code that assigns arint()result toresult->timewithout checking for NaN or int64 overflow. Division by a very small factor could produce values exceeding int64 range.♻️ Suggested change for consistency
/* cascade units down */ result->day += (int32) month_remainder_days; `#ifdef` HAVE_INT64_TIMESTAMP - result->time = rint(span->time / factor + sec_remainder * USECS_PER_SEC); + result_double = rint(span->time / factor + sec_remainder * USECS_PER_SEC); + if (isnan(result_double) || !FLOAT8_FITS_IN_INT64(result_double)) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("interval out of range"))); + result->time = (int64) result_double; `#else`This would require declaring
result_doubleat the top ofdsinterval_div.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/ivorysql_ora/src/datatype/dsinterval.c` around lines 2414 - 2419, dsinterval_div currently assigns rint(span->time / factor + sec_remainder * USECS_PER_SEC) directly to result->time under HAVE_INT64_TIMESTAMP without checking for NaN or int64 overflow; mirror the dsinterval_mul pattern by declaring a double (e.g., result_double) at the top of dsinterval_div, compute result_double = span->time / factor + sec_remainder * USECS_PER_SEC, then validate with isfinite(result_double) and that result_double is within PG_INT64_MIN..PG_INT64_MAX before assigning result->time = rint(result_double); if the value is non-finite or out of range, clamp result->time to PG_INT64_MIN or PG_INT64_MAX (matching dsinterval_mul behavior) to avoid undefined overflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contrib/ivorysql_ora/src/datatype/dsinterval.c`:
- Around line 2414-2419: dsinterval_div currently assigns rint(span->time /
factor + sec_remainder * USECS_PER_SEC) directly to result->time under
HAVE_INT64_TIMESTAMP without checking for NaN or int64 overflow; mirror the
dsinterval_mul pattern by declaring a double (e.g., result_double) at the top of
dsinterval_div, compute result_double = span->time / factor + sec_remainder *
USECS_PER_SEC, then validate with isfinite(result_double) and that result_double
is within PG_INT64_MIN..PG_INT64_MAX before assigning result->time =
rint(result_double); if the value is non-finite or out of range, clamp
result->time to PG_INT64_MIN or PG_INT64_MAX (matching dsinterval_mul behavior)
to avoid undefined overflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 91ae9c9a-d22f-4112-81b5-561637abd5ef
📒 Files selected for processing (1)
contrib/ivorysql_ora/src/datatype/dsinterval.c
dsinterval_div() in HAVE_INT64_TIMESTAMP mode assigned a rounded double directly to result->time without validating NaN/range. That can produce undefined overflow behavior on extreme inputs. Mirror dsinterval_mul() by validating the rounded value with FLOAT8_FITS_IN_INT64() before casting, and raise interval out of range when it is not representable. Add regression cases in ora_datetime for both overflow directions: interval * huge_number and interval / tiny_number, each expecting "interval out of range".
3abcbab to
9cca17c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contrib/ivorysql_ora/src/datatype/dsinterval.c (1)
2273-2274: Apply the same NaN-safety improvements toyminterval_mul,mul_d_yminterval, andyminterval_div.Both
yminterval_mulandmul_d_ymintervaluse the bounds-check patternif (result_double > INT_MAX || result_double < INT_MIN)without NaN validation, which is vulnerable to the same overflow bypass issue. Additionally,yminterval_divperforms a direct cast(int32) (span->month / factor)without any overflow or NaN protection.For consistency with
dsinterval_muland to maintain robustness, update these functions to checkisnan(result_double) || !FLOAT8_FITS_IN_INT32(result_double)before casting to int32.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/ivorysql_ora/src/datatype/dsinterval.c` around lines 2273 - 2274, Replace the existing fragile bounds checks in yminterval_mul, mul_d_yminterval, and yminterval_div with the same NaN-safe pattern used in dsinterval_mul: compute result_double, then validate with isnan(result_double) || !FLOAT8_FITS_IN_INT32(result_double) before casting to int32 (or returning); in particular, change any checks like (result_double > INT_MAX || result_double < INT_MIN) to the combined isnan + FLOAT8_FITS_IN_INT32 check, and for yminterval_div add this validation before the direct cast (int32)(span->month / factor).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contrib/ivorysql_ora/src/datatype/dsinterval.c`:
- Around line 2273-2274: Replace the existing fragile bounds checks in
yminterval_mul, mul_d_yminterval, and yminterval_div with the same NaN-safe
pattern used in dsinterval_mul: compute result_double, then validate with
isnan(result_double) || !FLOAT8_FITS_IN_INT32(result_double) before casting to
int32 (or returning); in particular, change any checks like (result_double >
INT_MAX || result_double < INT_MIN) to the combined isnan + FLOAT8_FITS_IN_INT32
check, and for yminterval_div add this validation before the direct cast
(int32)(span->month / factor).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f2e1ac3-e266-4a35-bbda-8056f769cd1c
📒 Files selected for processing (3)
contrib/ivorysql_ora/expected/ora_datetime.outcontrib/ivorysql_ora/sql/ora_datetime.sqlcontrib/ivorysql_ora/src/datatype/dsinterval.c
yminterval_mul() and mul_d_yminterval() still used manual INT_MAX/ INT_MIN checks on double intermediates, and yminterval_div() cast the division result directly to int32 without representability validation. Switch all three paths to the same NaN-safe pattern used in interval arithmetic: reject isnan() and !FLOAT8_FITS_IN_INT32() before casting. Add regression cases in ora_datetime for year-to-month interval overflow on both multiply (huge factor) and divide (tiny factor), each expecting "interval out of range".
dsinterval_mul() still validated month/day intermediates with manual INT_MAX/INT_MIN comparisons. That style does not reject NaN reliably and diverges from the FLOAT8_FITS_IN_INT32 pattern now used in related interval arithmetic paths. Switch month/day checks to isnan() || !FLOAT8_FITS_IN_INT32() before casting to int32. Add an ora_datetime regression case for interval day-to-second multiplication by NaN and expect "interval out of range".
9cca17c to
971560b
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contrib/ivorysql_ora/src/datatype/dsinterval.c (2)
2390-2428:⚠️ Potential issue | 🔴 CriticalAdd NaN and range checks for month/day divisions in
dsinterval_div()before the(int32)casts.Lines 2404–2405 cast division results directly to
int32without first checking for NaN or range overflow. This leaves the function vulnerable to undefined behavior when dividing byNaNor values that produce out-of-range quotients. The newly addedresult_doubleandFLOAT8_FITS_IN_INT64()guard on line 2425 protects onlyresult->time, notmonthorday.Apply the same guarding pattern already used in
yminterval_div(): compute the division result into adouble, check withisnan()andFLOAT8_FITS_IN_INT32(), then cast toint32.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/ivorysql_ora/src/datatype/dsinterval.c` around lines 2390 - 2428, In dsinterval_div(), avoid casting span->month/factor and span->day/factor directly to int32; instead compute each division into a double (reuse result_double), check isnan() and FLOAT8_FITS_IN_INT32() for the month and day results (like the existing check used for result->time), then cast to (int32) only after the checks; update the assignments that set result->month and result->day to perform this safe double->int32 guarded conversion and use the same error handling (ERRCODE_DATETIME_VALUE_OUT_OF_RANGE) if checks fail.
2341-2346:⚠️ Potential issue | 🟠 MajorMissing int32 overflow check for
result->dayafter time-to-day carry normalization.The overflow validation at lines 2343-2346 and 2423-2428 only ensures
result->timefits in int64. It does not validate that the subsequent carry operation at lines 2356 and 2439—result->day += result->time / USECS_PER_DAY—keepsresult->daywithin int32 bounds. For example,interval '21 23:59:59.999999' day to second * 99000000passes the int64 check but the day carry causes result->day to exceed INT32_MAX.This also applies to
dsinterval_divat lines 2423-2428 and 2439.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/ivorysql_ora/src/datatype/dsinterval.c` around lines 2341 - 2346, After normalizing time to microseconds and performing the carry into days (the operation result->day += result->time / USECS_PER_DAY in functions handling multiplication and division—e.g., dsinterval_mul and dsinterval_div), add an explicit int32 bounds check for result->day against INT32_MIN and INT32_MAX and ereport(ERROR, errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), errmsg("interval out of range")) if the carry makes result->day overflow; keep the existing int64 check for result->time but validate result->day immediately after the carry to prevent examples like large multipliers causing day overflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/ivorysql_ora/sql/ora_datetime.sql`:
- Around line 570-592: Add a missing division-by-NaN test for the day-to-second
interval: insert the SQL line "interval '365 11:11:11' day(3) to second /
'NaN'::number" alongside the other interval division/multiplication cases so the
dsinterval_div() NaN path is covered, and add the corresponding "ERROR:
interval out of range" entry to the expected output file for ora_datetime tests
to match the new failing case.
---
Outside diff comments:
In `@contrib/ivorysql_ora/src/datatype/dsinterval.c`:
- Around line 2390-2428: In dsinterval_div(), avoid casting span->month/factor
and span->day/factor directly to int32; instead compute each division into a
double (reuse result_double), check isnan() and FLOAT8_FITS_IN_INT32() for the
month and day results (like the existing check used for result->time), then cast
to (int32) only after the checks; update the assignments that set result->month
and result->day to perform this safe double->int32 guarded conversion and use
the same error handling (ERRCODE_DATETIME_VALUE_OUT_OF_RANGE) if checks fail.
- Around line 2341-2346: After normalizing time to microseconds and performing
the carry into days (the operation result->day += result->time / USECS_PER_DAY
in functions handling multiplication and division—e.g., dsinterval_mul and
dsinterval_div), add an explicit int32 bounds check for result->day against
INT32_MIN and INT32_MAX and ereport(ERROR,
errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), errmsg("interval out of range"))
if the carry makes result->day overflow; keep the existing int64 check for
result->time but validate result->day immediately after the carry to prevent
examples like large multipliers causing day overflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7dda8807-d344-4cf6-9a4f-b6219585abcb
📒 Files selected for processing (4)
contrib/ivorysql_ora/expected/ora_datetime.outcontrib/ivorysql_ora/sql/ora_datetime.sqlcontrib/ivorysql_ora/src/datatype/dsinterval.ccontrib/ivorysql_ora/src/datatype/yminterval.c
| -- interval year to month * huge number (overflow) | ||
| SELECT interval'10-11' year to month * 100000000000000000000; | ||
|
|
||
| -- interval day to second * number | ||
| SELECT interval'365 11:11:11' day(3) to second * 12.34; | ||
|
|
||
| -- interval day to second * huge number (overflow) | ||
| SELECT interval'365 11:11:11' day(3) to second * 100000000000000000000; | ||
|
|
||
| -- interval day to second * NaN number (overflow) | ||
| SELECT interval'365 11:11:11' day(3) to second * 'NaN'::number; | ||
|
|
||
| -- interval year to month / number | ||
| SELECT interval'10-11' year to month / 12.34; | ||
|
|
||
| -- interval year to month / tiny number (overflow) | ||
| SELECT interval'10-11' year to month / 0.00000000000000000001; | ||
|
|
||
| -- interval day to second / number | ||
| SELECT interval'365 11:11:11' day(3) to second / 12.34; | ||
|
|
||
| -- interval day to second / tiny number (overflow) | ||
| SELECT interval'365 11:11:11' day(3) to second / 0.00000000000000000001; |
There was a problem hiding this comment.
Add the missing division-by-NaN regression.
This block covers NaN only on multiplication, even though dsinterval_div() is also touched in this PR. Please add interval '365 11:11:11' day(3) to second / 'NaN'::number here and the matching ERROR: interval out of range entry in contrib/ivorysql_ora/expected/ora_datetime.out so the remaining NaN path stays covered.
As per coding guidelines, **/sql/*.sql: Test SQL files. Ensure comprehensive coverage of features.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contrib/ivorysql_ora/sql/ora_datetime.sql` around lines 570 - 592, Add a
missing division-by-NaN test for the day-to-second interval: insert the SQL line
"interval '365 11:11:11' day(3) to second / 'NaN'::number" alongside the other
interval division/multiplication cases so the dsinterval_div() NaN path is
covered, and add the corresponding "ERROR: interval out of range" entry to the
expected output file for ora_datetime tests to match the new failing case.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/ivorysql_ora/sql/ora_datetime.sql`:
- Around line 579-592: Add the three missing NaN test cases that exercise the
isnan() branches in the interval arithmetic functions: add SELECT interval
'10-11' year to month * 'NaN'::number to hit yminterval_mul, SELECT interval
'10-11' year to month / 'NaN'::number to hit yminterval_div, and SELECT interval
'365 11:11:11' day(3) to second / 'NaN'::number to hit dsinterval_div; place
them alongside the existing interval tests so yminterval_mul, yminterval_div,
dsinterval_mul, and dsinterval_div all have NaN coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b17f001-173f-460f-8492-db6dfff810dd
📒 Files selected for processing (4)
contrib/ivorysql_ora/expected/ora_datetime.outcontrib/ivorysql_ora/sql/ora_datetime.sqlcontrib/ivorysql_ora/src/datatype/dsinterval.ccontrib/ivorysql_ora/src/datatype/yminterval.c
| -- interval day to second * NaN number (overflow) | ||
| SELECT interval'365 11:11:11' day(3) to second * 'NaN'::number; | ||
|
|
||
| -- interval year to month / number | ||
| SELECT interval'10-11' year to month / 12.34; | ||
|
|
||
| -- interval year to month / tiny number (overflow) | ||
| SELECT interval'10-11' year to month / 0.00000000000000000001; | ||
|
|
||
| -- interval day to second / number | ||
| SELECT interval'365 11:11:11' day(3) to second / 12.34; | ||
|
|
||
| -- interval day to second / tiny number (overflow) | ||
| SELECT interval'365 11:11:11' day(3) to second / 0.00000000000000000001; |
There was a problem hiding this comment.
Add missing NaN tests for complete coverage.
The tests cover NaN multiplication for day-to-second intervals but are missing:
interval '10-11' year to month * 'NaN'::number- testsyminterval_mulNaN pathinterval '10-11' year to month / 'NaN'::number- testsyminterval_divNaN pathinterval '365 11:11:11' day(3) to second / 'NaN'::number- testsdsinterval_divNaN path (also noted in a prior review)
Since all four arithmetic functions (yminterval_mul, yminterval_div, dsinterval_mul, dsinterval_div) now include isnan() checks, they should all have corresponding test coverage.
As per coding guidelines, **/sql/*.sql: Test SQL files. Ensure comprehensive coverage of features.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contrib/ivorysql_ora/sql/ora_datetime.sql` around lines 579 - 592, Add the
three missing NaN test cases that exercise the isnan() branches in the interval
arithmetic functions: add SELECT interval '10-11' year to month * 'NaN'::number
to hit yminterval_mul, SELECT interval '10-11' year to month / 'NaN'::number to
hit yminterval_div, and SELECT interval '365 11:11:11' day(3) to second /
'NaN'::number to hit dsinterval_div; place them alongside the existing interval
tests so yminterval_mul, yminterval_div, dsinterval_mul, and dsinterval_div all
have NaN coverage.
|
@jiaoshuntian follow up this PR |
|
in dsinterval_div(): |
dsinterval_mul() compared a rounded double directly to PG_INT64_MAX, which triggers Clang because that integer constant is not exactly representable as double near 2^63.
Warning excerpt:
Follow the same integer-fit pattern used in timestamp.c by checking isnan() and FLOAT8_FITS_IN_INT64() after rint(). This avoids custom bound constants and keeps overflow checks consistent with core code.
Summary by CodeRabbit
Bug Fixes
Tests
Style / Refactor