Conversation
📝 WalkthroughWalkthroughAdds an Oracle-compatible ASCII() implementation: a new C function Changes
Sequence Diagram(s)(omitted — changes are not a multi-component control-flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@contrib/ivorysql_ora/expected/ora_ascii.out`:
- Around line 35-54: The test uses string literals so it exercises the varchar
overload instead of the date/time overloads; update the test expectations to
call ascii with real date/time types to exercise ora_ascii's
oradate/oratimestamp/oratimestamptz paths by replacing or adding the selects to
use to_date('01-JAN-2026 01:02:03','DD-MON-YYYY HH24:MI:SS'),
to_timestamp('11-JAN-2026 01:02:03.00','DD-MON-YYYY HH24:MI:SS.FF'), and
to_timestamp_tz('22-JAN-2026 01:02:03.00 +01:00','DD-MON-YYYY HH24:MI:SS.FF
TZH:TZM') when invoking ascii so the tests target the ora_ascii overloads for
oradate/oratimestamp/oratimestamptz.
In `@contrib/ivorysql_ora/sql/ora_ascii.sql`:
- Around line 15-22: The NLS format settings are unused because the queries pass
string literals to ora_ascii; change each select to pass actual date/time values
so NLS is applied — for the date test call ora_ascii(to_date('01-JAN-2026
01:02:03', 'DD-MON-YYYY HH24:MI:SS')), for the timestamp test call
ora_ascii(to_timestamp('11-JAN-2026 01:02:03.00', 'DD-MON-YYYY
HH24:MI:SS.FF6')), and for the timestamp with TZ test call
ora_ascii(to_timestamp_tz('22-JAN-2026 01:02:03.00 +01:00', 'DD-MON-YYYY
HH24:MI:SS.FF6 TZH:TZM')); keep the set nls_* lines as-is so the conversion
performed by ora_ascii on date/timestamp/timestamptz values will honor the
configured formats.
In `@contrib/ivorysql_ora/src/builtin_functions/builtin_functions--1.0.sql`:
- Around line 1421-1443: Change the three function declarations for sys.ascii
that are currently declared IMMUTABLE to STABLE: update the definitions of
sys.ascii(sys.oradate), sys.ascii(sys.oratimestamp), and
sys.ascii(sys.oratimestamptz) so they use STABLE instead of IMMUTABLE (leave the
rest of the attributes like LANGUAGE C, STRICT, and PARALLEL SAFE as-is unless
further review requires adjustment).
In `@contrib/ivorysql_ora/src/builtin_functions/character_datatype_functions.c`:
- Around line 2364-2368: The NUMBEROID branch incorrectly uses
PG_GETARG_INT32(0); replace that with retrieving the value as a PostgreSQL
Numeric (e.g., use PG_GETARG_NUMERIC(0) or DatumGetNumeric(PG_GETARG_DATUM(0)))
and convert it to a string with the numeric output routine (numeric_out /
numeric_to_cstring or equivalent) so large values and fractional parts are
preserved; update the case for NUMBEROID to obtain a Numeric and call the proper
numeric->cstring conversion instead of casting to int32.
- Around line 2370-2379: The cases for BINARY_FLOATOID and BINARY_DOUBLEOID
incorrectly format values using psprintf("%f", val), producing fixed-point
output; replace those sprintf calls by invoking the existing binary_float_out
and binary_double_out functions to obtain the Oracle-compatible shortest-decimal
strings—retrieve the argument with PG_GETARG_FLOAT4(0)/PG_GETARG_FLOAT8(0) as
done now, call binary_float_out()/binary_double_out() to get the proper C
string/result, and assign that result to str instead of using psprintf.
- Around line 2395-2398: Replace the GetConfigOptionByName-based defensive
pattern with direct use of the registered GUC extern variables: remove calls to
GetConfigOptionByName("nls_date_format", ...),
GetConfigOptionByName("nls_timestamp_format", ...), and
GetConfigOptionByName("nls_timestamp_tz_format", ...) and simply read the extern
variables nls_date_format, nls_timestamp_format, and nls_timestamp_tz_format
directly (they have defaults set to "YYYY-MM-DD" etc.), mirroring usage in
oradate.c and datetime_datatype_functions.c so the NULL checks and fallback
assignments are eliminated.
🧹 Nitpick comments (1)
contrib/ivorysql_ora/expected/ora_ascii.out (1)
1-55: Consider adding edge case tests for comprehensive coverage.The current tests cover basic scenarios but miss important edge cases:
- Empty string: What does
ascii('')return? (Oracle returns NULL)- Multi-byte characters: Test with UTF-8 input like
ascii('日本')to verify first-character behavior- Negative numbers: Test
ascii(-123)to verify the '-' character is handled- Zero: Test
ascii(0)- Decimal with leading zero: Test
ascii(0.5f)orascii(0.5d)These edge cases would help ensure Oracle-compatible behavior.
| set nls_date_format='DD-MON-YYYY HH24:MI:SS'; | ||
| select ascii('01-JAN-2026 01:02:03') from dual; | ||
| ascii | ||
| ------- | ||
| 48 | ||
| (1 row) | ||
|
|
||
| set nls_timestamp_format='DD-MON-YYYY HH24:MI:SS.FF6'; | ||
| select ascii('11-JAN-2026 01:02:03.00') from dual; | ||
| ascii | ||
| ------- | ||
| 49 | ||
| (1 row) | ||
|
|
||
| set nls_timestamp_tz_format='DD-MON-YYYY HH24:MI:SS TZH:TZM'; | ||
| select ascii('22-JAN-2026 01:02:03.00 +01:00') from dual; | ||
| ascii | ||
| ------- | ||
| 50 | ||
| (1 row) |
There was a problem hiding this comment.
Test coverage gap: date/time type overloads are not exercised.
The tests for date/time inputs use string literals (e.g., '01-JAN-2026 01:02:03'), which will be resolved to the sys.ascii(sys.oravarcharchar) overload rather than the sys.ascii(sys.oradate), sys.ascii(sys.oratimestamp), or sys.ascii(sys.oratimestamptz) overloads.
To properly test the date/time type conversions implemented in ora_ascii, consider adding tests that explicitly use date/time type inputs:
-- Test actual oradate type
select ascii(to_date('01-JAN-2026 01:02:03', 'DD-MON-YYYY HH24:MI:SS')) from dual;
-- Test actual oratimestamp type
select ascii(to_timestamp('11-JAN-2026 01:02:03.00', 'DD-MON-YYYY HH24:MI:SS.FF')) from dual;
-- Test actual oratimestamptz type
select ascii(to_timestamp_tz('22-JAN-2026 01:02:03.00 +01:00', 'DD-MON-YYYY HH24:MI:SS.FF TZH:TZM')) from dual;As per coding guidelines, these expected-output files should include edge case coverage for Oracle compatibility behavior.
🤖 Prompt for AI Agents
In `@contrib/ivorysql_ora/expected/ora_ascii.out` around lines 35 - 54, The test
uses string literals so it exercises the varchar overload instead of the
date/time overloads; update the test expectations to call ascii with real
date/time types to exercise ora_ascii's oradate/oratimestamp/oratimestamptz
paths by replacing or adding the selects to use to_date('01-JAN-2026
01:02:03','DD-MON-YYYY HH24:MI:SS'), to_timestamp('11-JAN-2026
01:02:03.00','DD-MON-YYYY HH24:MI:SS.FF'), and to_timestamp_tz('22-JAN-2026
01:02:03.00 +01:00','DD-MON-YYYY HH24:MI:SS.FF TZH:TZM') when invoking ascii so
the tests target the ora_ascii overloads for
oradate/oratimestamp/oratimestamptz.
| set nls_date_format='DD-MON-YYYY HH24:MI:SS'; | ||
| select ascii('01-JAN-2026 01:02:03') from dual; | ||
|
|
||
| set nls_timestamp_format='DD-MON-YYYY HH24:MI:SS.FF6'; | ||
| select ascii('11-JAN-2026 01:02:03.00') from dual; | ||
|
|
||
| set nls_timestamp_tz_format='DD-MON-YYYY HH24:MI:SS TZH:TZM'; | ||
| select ascii('22-JAN-2026 01:02:03.00 +01:00') from dual; |
There was a problem hiding this comment.
The NLS settings are set but not utilized by the subsequent queries.
Setting nls_date_format, nls_timestamp_format, and nls_timestamp_tz_format has no effect on the following queries because they use string literals ('01-JAN-2026 01:02:03'), which are parsed as oravarcharchar type, not as date/time types.
To actually test the NLS format behavior with the ora_ascii function on date/time types, you would need to pass actual date/time values:
set nls_date_format='DD-MON-YYYY HH24:MI:SS';
select ascii(to_date('01-JAN-2026 01:02:03', 'DD-MON-YYYY HH24:MI:SS')) from dual;This would ensure the ora_ascii function's date-to-string conversion respects the NLS format.
🤖 Prompt for AI Agents
In `@contrib/ivorysql_ora/sql/ora_ascii.sql` around lines 15 - 22, The NLS format
settings are unused because the queries pass string literals to ora_ascii;
change each select to pass actual date/time values so NLS is applied — for the
date test call ora_ascii(to_date('01-JAN-2026 01:02:03', 'DD-MON-YYYY
HH24:MI:SS')), for the timestamp test call ora_ascii(to_timestamp('11-JAN-2026
01:02:03.00', 'DD-MON-YYYY HH24:MI:SS.FF6')), and for the timestamp with TZ test
call ora_ascii(to_timestamp_tz('22-JAN-2026 01:02:03.00 +01:00', 'DD-MON-YYYY
HH24:MI:SS.FF6 TZH:TZM')); keep the set nls_* lines as-is so the conversion
performed by ora_ascii on date/timestamp/timestamptz values will honor the
configured formats.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@contrib/ivorysql_ora/src/builtin_functions/character_datatype_functions.c`:
- Around line 2395-2428: The local uninitialized pointers nls_date_format,
nls_timestamp_format, and nls_timestamp_tz_format in the switch cases shadow the
GUC externs and are dereferenced, causing crashes; remove those local
declarations so the code uses the global GUC variables defined in guc.h (the
existing includes permit this), then keep the calls to
cstring_to_text(nls_date_format) / cstring_to_text(nls_timestamp_format) /
cstring_to_text(nls_timestamp_tz_format) as-is (used with timestamp_to_char,
timestamptz_to_char) so the functions TimestampGetDatum/TimestampTzGetDatum and
DirectFunctionCall2 use the proper initialized globals.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@contrib/ivorysql_ora/src/builtin_functions/character_datatype_functions.c`:
- Around line 2434-2437: The ASCII implementation in
character_datatype_functions.c currently returns 0 for empty strings; change the
empty-string handling in the ASCII function so that if str == NULL || str[0] ==
'\0' it returns NULL (use PG_RETURN_NULL()) instead of PG_RETURN_INT32(0), and
keep the existing PG_RETURN_INT32((unsigned char) str[0]) for non-empty strings;
also add a regression SQL test asserting SELECT ASCII('') IS NULL to verify
Oracle-compatible behavior.
| if (str == NULL || str[0] == '\0') | ||
| PG_RETURN_INT32(0); | ||
|
|
||
| PG_RETURN_INT32((unsigned char) str[0]); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Oracle ASCII function behavior empty string NULL semantics
💡 Result:
In Oracle SQL, an empty character string ('') is treated as NULL (“a character value with a length of zero as null”). Because of that, any function call that receives '' is effectively receiving NULL. [1]
ASCII(char) returns the decimal representation (in the database character set) of the first character of char. [2] Therefore:
SELECT ASCII('') AS ascii_empty,
ASCII(NULL) AS ascii_null
FROM dual;Both expressions return NULL, because '' becomes NULL in Oracle SQL. [1]
Notes:
- The value depends on your DB character set (ASCII vs EBCDIC, etc.). [2]
- Oracle documents that the “empty string = NULL” behavior might change in a future release, so relying on it is discouraged. [1]
Sources: [1] [2]
🏁 Script executed:
# First, let's examine the file and surrounding context
cd contrib/ivorysql_ora/src/builtin_functions
cat -n character_datatype_functions.c | sed -n '2420,2450p'Repository: IvorySQL/IvorySQL
Length of output: 669
🏁 Script executed:
# Check for related tests or documentation about ASCII function
fd -t f '.*test.*' --search-path contrib/ivorysql_ora --exec grep -l 'ascii\|ASCII' {} \;Repository: IvorySQL/IvorySQL
Length of output: 43
🏁 Script executed:
# Look for any documentation or comments about empty string handling in the codebase
rg -i 'ascii.*empty|empty.*ascii' contrib/ivorysql_ora --type c -A 3 -B 3Repository: IvorySQL/IvorySQL
Length of output: 43
🏁 Script executed:
# Search for ASCII function tests more broadly
find . -path "*/test*" -name "*.sql" -o -name "*.pl" | head -20 | xargs grep -l -i "ascii" 2>/dev/nullRepository: IvorySQL/IvorySQL
Length of output: 110
🏁 Script executed:
# Check the complete function definition for context and any documentation
cat -n contrib/ivorysql_ora/src/builtin_functions/character_datatype_functions.c | sed -n '2380,2440p'Repository: IvorySQL/IvorySQL
Length of output: 2325
🏁 Script executed:
# Search for Oracle ASCII function documentation or comments in the codebase
rg -i "ascii.*function|function.*ascii" contrib/ivorysql_ora --type c -B 5 -A 5Repository: IvorySQL/IvorySQL
Length of output: 4859
🏁 Script executed:
# Get the function definition start
cat -n contrib/ivorysql_ora/src/builtin_functions/character_datatype_functions.c | sed -n '2340,2380p'Repository: IvorySQL/IvorySQL
Length of output: 1414
🏁 Script executed:
# Search for tests of the ASCII function
find . -path "*/test*" -type f \( -name "*.sql" -o -name "*.expected" \) | xargs grep -l "ora_ascii\|oracle.*ascii" 2>/dev/nullRepository: IvorySQL/IvorySQL
Length of output: 45
🏁 Script executed:
# Check if there are any comments or documentation about NULL/empty string handling
grep -n "PG_RETURN_INT32(0)" contrib/ivorysql_ora/src/builtin_functions/character_datatype_functions.cRepository: IvorySQL/IvorySQL
Length of output: 120
🏁 Script executed:
# Check for any test files that might validate the function behavior
find . -path "*/regress*" -o -path "*/test*" | grep -i "oracle\|ora" | head -20Repository: IvorySQL/IvorySQL
Length of output: 1237
🏁 Script executed:
# Look for any documentation in the repository about Oracle compatibility
find contrib/ivorysql_ora -name "*.md" -o -name "*.txt" -o -name "*.rst" | xargs cat 2>/dev/null | head -50Repository: IvorySQL/IvorySQL
Length of output: 131
🏁 Script executed:
# Check if there are any related issues or comments in the git history about ASCII function
git log --oneline --all -- contrib/ivorysql_ora/src/builtin_functions/character_datatype_functions.c | head -10Repository: IvorySQL/IvorySQL
Length of output: 206
Fix empty-string handling to match Oracle ASCII semantics.
Oracle treats empty strings as NULL, so ASCII('') returns NULL. The current implementation at lines 2434-2435 returns 0 for empty strings instead. Change the empty-string check to return NULL:
Suggested fix
if (str == NULL || str[0] == '\0')
PG_RETURN_NULL();
PG_RETURN_INT32((unsigned char) str[0]);Add a regression test to verify behavior: SELECT ASCII('') IS NULL should return true.
🤖 Prompt for AI Agents
In `@contrib/ivorysql_ora/src/builtin_functions/character_datatype_functions.c`
around lines 2434 - 2437, The ASCII implementation in
character_datatype_functions.c currently returns 0 for empty strings; change the
empty-string handling in the ASCII function so that if str == NULL || str[0] ==
'\0' it returns NULL (use PG_RETURN_NULL()) instead of PG_RETURN_INT32(0), and
keep the existing PG_RETURN_INT32((unsigned char) str[0]) for non-empty strings;
also add a regression SQL test asserting SELECT ASCII('') IS NULL to verify
Oracle-compatible behavior.
|
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. |
|
Since IvorySQL does not support the nvarchar2 data type, there is no need to implement type conversions related to nvarchar2. Your implementation and test cases are very thorough and well-considered. |
|
I have signed and sent the CLA this morning. |
Thank you for signing and sending the CLA. However, we haven’t received it yet at [email protected]. To help resolve this quickly, could you please:
We’ll monitor the inbox closely and confirm receipt as soon as it arrives. Let us know if you encounter any issues! Thanks for your contribution and patience! |
|
Hello, Yesterday I have used a wrong email address. Sorry for that. Thanks. |
We have successfully received it and will merge this PR. Thanks |
Add ASCII SQL function with automatic test.
I have tested with:
make installcheck REGRESS=ora_ascii
...
echo "# +++ regress install-check in contrib/ivorysql_ora +++" && ../../src/test/regress/pg_regress --inputdir=. --bindir='/home/ivory/local/ivorysql/ivorysql-5/bin' --dbname=contrib_regression ora_ascii
+++ regress install-check in contrib/ivorysql_ora +++
using postmaster on Unix socket, port 1521
ok 1 - ora_ascii 7 ms
1..1
All 1 tests passed.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.