Implement ALL_TAB_COLUMNS view and add test case#1290
Implement ALL_TAB_COLUMNS view and add test case#1290Adria0615 wants to merge 2 commits intoIvorySQL:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds an Oracle-compatible ALL_TAB_COLUMNS view and corresponding test that creates/drops a sample table and validates column metadata returned by the new view. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Script
participant DB as Database
participant View as ALL_TAB_COLUMNS View
participant Catalog as pg_catalog
Test->>DB: CREATE TABLE test_user_cons (...)
Test->>DB: SELECT ... FROM ALL_TAB_COLUMNS WHERE TABLE_NAME='test_user_cons'
DB->>View: Evaluate ALL_TAB_COLUMNS query
View->>Catalog: Read pg_class/pg_attribute/pg_type/pg_namespace
Catalog-->>View: Return attribute/type metadata
View-->>DB: Return formatted Oracle-style rows
DB-->>Test: Query result with column metadata
Test->>DB: DROP TABLE test_user_cons
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 SQLFluff (4.1.0)contrib/ivorysql_ora/sql/ora_sysview.sqlUser Error: Specified path does not exist. Check it/they exist(s): contrib/ivorysql_ora/sql/ora_sysview.sql. 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: 2
🧹 Nitpick comments (3)
contrib/ivorysql_ora/sql/ora_sysview.sql (2)
194-195: Clarify intent of syntax error test.The backtick after
END;appears intentional to test syntax error handling. However, this conflicts with the earlier version of the same test at lines 67-79 which creates the function successfully.If testing error scenarios is intended, consider adding a comment explaining this is a negative test case, or use a distinct function name to avoid confusion with the earlier successful test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/ivorysql_ora/sql/ora_sysview.sql` around lines 194 - 195, The test contains a stray backtick after "END;" (i.e., "END;`") which appears to be an intentional negative syntax test but conflicts with an earlier successful function creation; either (1) add an inline comment immediately before the "END;`" indicating this is a negative/intentional syntax-error test, or (2) change the function name used here to a distinct name (different from the earlier successful test) so it's clear this block intentionally verifies syntax error handling; update the surrounding text to make the intent explicit.
217-229: Test queries PostgreSQL types instead of Oracle types.The table is initially created with Oracle-style types (
NUMBER PRIMARY KEY,VARCHAR2(50)), then dropped and recreated with PostgreSQL types (int primary key,varchar(50)) before queryingALL_TAB_COLUMNS.To properly test Oracle compatibility, consider querying
ALL_TAB_COLUMNSagainst the Oracle-typed table definition instead:Proposed fix to test Oracle types
-- Test ALL_TAB_COLUMNS DROP TABLE IF EXISTS test_user_cons; CREATE TABLE test_user_cons ( - id int primary key, - name varchar(50) not null + id NUMBER PRIMARY KEY, + name VARCHAR2(50) NOT NULL ); SELECT OWNER, TABLE_NAME, COLUMN_NAME, COLUMN_ID, DATA_TYPE, DATA_LENGTH, DATA_PRECISION, DATA_SCALE, NULLABLE FROM ALL_TAB_COLUMNS WHERE TABLE_NAME = 'test_user_cons';Note: The expected output file would need corresponding updates to reflect the Oracle type mappings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/ivorysql_ora/sql/ora_sysview.sql` around lines 217 - 229, The test creates test_user_cons using PostgreSQL types (int, varchar) but is meant to exercise Oracle mappings via ALL_TAB_COLUMNS; change the CREATE TABLE for test_user_cons to use Oracle-style types (e.g., NUMBER PRIMARY KEY and VARCHAR2(50) NOT NULL), keep the DROP TABLE IF EXISTS test_user_cons and the subsequent SELECT against ALL_TAB_COLUMNS as-is, and update the expected output file to reflect Oracle type mappings returned by ALL_TAB_COLUMNS.contrib/ivorysql_ora/expected/ora_sysview.out (1)
330-334: Consider addingoravarcharbyteto thedata_lengthcalculation in the ALL_TAB_COLUMNS view for improved Oracle compatibility.The view currently returns
DATA_LENGTH = -1fororavarcharbytecolumns because the type is not in the special case list that extracts length fromatttypmod. Sinceoravarcharbytehastypmodin/typmodoutfunctions registered and atttypmod is populated for this type (measured in bytes), adding logic similar to the existingvarcharhandling would return the actual column length (e.g., 50 forVARCHAR2(50)) instead of-1, matching Oracle's behavior. This would require updating the view definition and the corresponding expected test output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/ivorysql_ora/expected/ora_sysview.out` around lines 330 - 334, The ALL_TAB_COLUMNS view currently treats oravarcharbyte as a special-case missing from the atttypmod-based length extraction, causing DATA_LENGTH = -1; update the view's data_length expression (the same logic used for varchar/varlena typmod extraction) to include oravarcharbyte in the branch that computes length from atttypmod, using atttypmod (measured in bytes) and existing typmod decoding logic so that columns of type oravarcharbyte return the actual length (e.g., 50) instead of -1; adjust the view definition for ALL_TAB_COLUMNS and regenerate the expected test output accordingly.
🤖 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/src/sysview/sysview--1.0.sql`:
- Around line 1310-1311: Update the GRANT and COMMENT statements to reference
the view with its schema-qualified name; change the unqualified references of
ALL_TAB_COLUMNS to SYS.ALL_TAB_COLUMNS in the GRANT SELECT ON and COMMENT ON
VIEW lines so they operate on the SYS schema view (i.e., update the statements
that currently read "GRANT SELECT ON ALL_TAB_COLUMNS TO PUBLIC;" and "COMMENT ON
VIEW ALL_TAB_COLUMNS ..." to use SYS.ALL_TAB_COLUMNS).
- Around line 1279-1308: Rename the view to be created as SYS.ALL_TAB_COLUMNS
(not unqualified ALL_TAB_COLUMNS); wrap owner, table_name and column_name
expressions with SYS.ORA_CASE_TRANS() to match Oracle identifier casing behavior
(replace n.nspname, c.relname, a.attname with SYS.ORA_CASE_TRANS(n.nspname) etc.
and adjust the ORDER BY accordingly); and add the same privilege filtering used
by other ALL_* views (e.g., include the schema/table visibility check using
HAS_SCHEMA_PRIVILEGE/appropriate HAS_* privilege calls on n.nspname/c.oid so
only accessible columns are returned). Ensure references to
pg_class/pg_attribute/pg_type/pg_namespace remain the same and keep the relkind
and att filters.
---
Nitpick comments:
In `@contrib/ivorysql_ora/expected/ora_sysview.out`:
- Around line 330-334: The ALL_TAB_COLUMNS view currently treats oravarcharbyte
as a special-case missing from the atttypmod-based length extraction, causing
DATA_LENGTH = -1; update the view's data_length expression (the same logic used
for varchar/varlena typmod extraction) to include oravarcharbyte in the branch
that computes length from atttypmod, using atttypmod (measured in bytes) and
existing typmod decoding logic so that columns of type oravarcharbyte return the
actual length (e.g., 50) instead of -1; adjust the view definition for
ALL_TAB_COLUMNS and regenerate the expected test output accordingly.
In `@contrib/ivorysql_ora/sql/ora_sysview.sql`:
- Around line 194-195: The test contains a stray backtick after "END;" (i.e.,
"END;`") which appears to be an intentional negative syntax test but conflicts
with an earlier successful function creation; either (1) add an inline comment
immediately before the "END;`" indicating this is a negative/intentional
syntax-error test, or (2) change the function name used here to a distinct name
(different from the earlier successful test) so it's clear this block
intentionally verifies syntax error handling; update the surrounding text to
make the intent explicit.
- Around line 217-229: The test creates test_user_cons using PostgreSQL types
(int, varchar) but is meant to exercise Oracle mappings via ALL_TAB_COLUMNS;
change the CREATE TABLE for test_user_cons to use Oracle-style types (e.g.,
NUMBER PRIMARY KEY and VARCHAR2(50) NOT NULL), keep the DROP TABLE IF EXISTS
test_user_cons and the subsequent SELECT against ALL_TAB_COLUMNS as-is, and
update the expected output file to reflect Oracle type mappings returned by
ALL_TAB_COLUMNS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3313e89b-2aaf-429f-bf58-53c9b150253a
📒 Files selected for processing (3)
contrib/ivorysql_ora/expected/ora_sysview.outcontrib/ivorysql_ora/sql/ora_sysview.sqlcontrib/ivorysql_ora/src/sysview/sysview--1.0.sql
| CREATE OR REPLACE VIEW ALL_TAB_COLUMNS AS | ||
| SELECT | ||
| n.nspname AS owner, | ||
| c.relname AS table_name, | ||
| a.attname AS column_name, | ||
| a.attnum AS column_id, | ||
| t.typname AS data_type, | ||
| CASE | ||
| WHEN t.typname IN ('varchar', 'char', 'bpchar') THEN (a.atttypmod - 4) | ||
| WHEN t.typname = 'numeric' THEN ((a.atttypmod - 4) >> 16) | ||
| ELSE a.attlen | ||
| END AS data_length, | ||
| CASE | ||
| WHEN t.typname = 'numeric' THEN ((a.atttypmod - 4) >> 16) | ||
| ELSE NULL | ||
| END AS data_precision, | ||
| CASE | ||
| WHEN t.typname = 'numeric' THEN ((a.atttypmod - 4) & 65535) | ||
| ELSE NULL | ||
| END AS data_scale, | ||
| CASE WHEN a.attnotnull THEN 'N' ELSE 'Y' END AS nullable | ||
| FROM pg_class c | ||
| JOIN pg_attribute a ON a.attrelid = c.oid | ||
| JOIN pg_type t ON a.atttypid = t.oid | ||
| JOIN pg_namespace n ON n.oid = c.relnamespace | ||
| WHERE a.attnum > 0 | ||
| AND NOT a.attisdropped | ||
| AND c.relkind IN ('r', 'v') | ||
| AND n.nspname NOT IN ('pg_catalog', 'information_schema') | ||
| ORDER BY n.nspname, c.relname, a.attnum; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing SYS. schema prefix for consistency with other views.
All other views in this file are created under the SYS schema (e.g., SYS.ALL_PROCEDURES, SYS.ALL_SOURCE, SYS.ALL_ARGUMENTS), but ALL_TAB_COLUMNS is created without the schema prefix. Additionally:
- Missing
SYS.ORA_CASE_TRANS()calls forowner,table_name, andcolumn_namecolumns to match Oracle identifier casing behavior - Missing privilege checks like
HAS_SCHEMA_PRIVILEGE()that otherALL_*views include
Proposed fix for schema and consistency
-CREATE OR REPLACE VIEW ALL_TAB_COLUMNS AS
+CREATE OR REPLACE VIEW SYS.ALL_TAB_COLUMNS AS
SELECT
- n.nspname AS owner,
- c.relname AS table_name,
- a.attname AS column_name,
+ SYS.ORA_CASE_TRANS(n.nspname::VARCHAR2)::VARCHAR2(128) AS owner,
+ SYS.ORA_CASE_TRANS(c.relname::VARCHAR2)::VARCHAR2(128) AS table_name,
+ SYS.ORA_CASE_TRANS(a.attname::VARCHAR2)::VARCHAR2(128) AS column_name,
a.attnum AS column_id,
t.typname AS data_type,
...
WHERE a.attnum > 0
AND NOT a.attisdropped
AND c.relkind IN ('r', 'v')
AND n.nspname NOT IN ('pg_catalog', 'information_schema')
+ AND HAS_SCHEMA_PRIVILEGE(n.oid, 'USAGE')
ORDER BY n.nspname, c.relname, a.attnum;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contrib/ivorysql_ora/src/sysview/sysview--1.0.sql` around lines 1279 - 1308,
Rename the view to be created as SYS.ALL_TAB_COLUMNS (not unqualified
ALL_TAB_COLUMNS); wrap owner, table_name and column_name expressions with
SYS.ORA_CASE_TRANS() to match Oracle identifier casing behavior (replace
n.nspname, c.relname, a.attname with SYS.ORA_CASE_TRANS(n.nspname) etc. and
adjust the ORDER BY accordingly); and add the same privilege filtering used by
other ALL_* views (e.g., include the schema/table visibility check using
HAS_SCHEMA_PRIVILEGE/appropriate HAS_* privilege calls on n.nspname/c.oid so
only accessible columns are returned). Ensure references to
pg_class/pg_attribute/pg_type/pg_namespace remain the same and keep the relkind
and att filters.
| GRANT SELECT ON ALL_TAB_COLUMNS TO PUBLIC; | ||
| COMMENT ON VIEW ALL_TAB_COLUMNS IS 'Oracle-style ALL_TAB_COLUMNS view'; No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Update GRANT to reference the correct schema.
If the view is moved to SYS schema as suggested above, the GRANT and COMMENT statements should also reference SYS.ALL_TAB_COLUMNS.
Proposed fix
-GRANT SELECT ON ALL_TAB_COLUMNS TO PUBLIC;
-COMMENT ON VIEW ALL_TAB_COLUMNS IS 'Oracle-style ALL_TAB_COLUMNS view';
+GRANT SELECT ON SYS.ALL_TAB_COLUMNS TO PUBLIC;
+COMMENT ON VIEW SYS.ALL_TAB_COLUMNS IS 'Oracle-style ALL_TAB_COLUMNS view';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GRANT SELECT ON ALL_TAB_COLUMNS TO PUBLIC; | |
| COMMENT ON VIEW ALL_TAB_COLUMNS IS 'Oracle-style ALL_TAB_COLUMNS view'; | |
| GRANT SELECT ON SYS.ALL_TAB_COLUMNS TO PUBLIC; | |
| COMMENT ON VIEW SYS.ALL_TAB_COLUMNS IS 'Oracle-style ALL_TAB_COLUMNS view'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contrib/ivorysql_ora/src/sysview/sysview--1.0.sql` around lines 1310 - 1311,
Update the GRANT and COMMENT statements to reference the view with its
schema-qualified name; change the unqualified references of ALL_TAB_COLUMNS to
SYS.ALL_TAB_COLUMNS in the GRANT SELECT ON and COMMENT ON VIEW lines so they
operate on the SYS schema view (i.e., update the statements that currently read
"GRANT SELECT ON ALL_TAB_COLUMNS TO PUBLIC;" and "COMMENT ON VIEW
ALL_TAB_COLUMNS ..." to use SYS.ALL_TAB_COLUMNS).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_sysview.sql`:
- Around line 97-100: Extend the test table definition used by the
ALL_TAB_COLUMNS coverage by adding columns that exercise numeric precision/scale
and nullable cases: keep the existing id (int4) and name (varchar) columns, then
add a numeric/decimal column with explicit precision and scale (e.g.,
NUMERIC(p,s)) to trigger precision/scale handling, and add at least one column
declared NULLABLE (no NOT NULL) to cover the NULLABLE = 'Y' path; update any
assertions in the test to expect the precision/scale values and nullable flag
for the new columns so ALL_TAB_COLUMNS branches are exercised (refer to
test_user_cons and the view logic that reads column_precision/column_scale and
nullable status).
- Around line 102-106: The SELECT against ALL_TAB_COLUMNS is only filtering by
TABLE_NAME ('test_user_cons') which can return rows from other schemas; update
the WHERE clause in the query that references ALL_TAB_COLUMNS to also restrict
OWNER (e.g., OWNER = current_schema or the expected schema name) so the
assertion is scoped to the intended schema and avoids cross-schema flakiness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0a2caac-ac9f-48a7-81e8-88651aa49ec3
📒 Files selected for processing (2)
contrib/ivorysql_ora/expected/ora_sysview.outcontrib/ivorysql_ora/sql/ora_sysview.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- contrib/ivorysql_ora/expected/ora_sysview.out
| CREATE TABLE test_user_cons ( | ||
| id int primary key, | ||
| name varchar(50) not null | ||
| ); |
There was a problem hiding this comment.
Expand this test to cover untested ALL_TAB_COLUMNS branches.
Current case validates only int4 and varchar with NOT NULL. It misses numeric precision/scale and NULLABLE = 'Y' paths implemented by the view.
Suggested expansion
CREATE TABLE test_user_cons (
id int primary key,
- name varchar(50) not null
+ name varchar(50) not null,
+ amount numeric(10,2),
+ code char(3)
); SELECT OWNER, TABLE_NAME, COLUMN_NAME, COLUMN_ID,
DATA_TYPE, DATA_LENGTH, DATA_PRECISION, DATA_SCALE,
NULLABLE
FROM ALL_TAB_COLUMNS
WHERE TABLE_NAME = 'test_user_cons'
AND OWNER = current_schema();
+ORDER BY COLUMN_ID;Also applies to: 102-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contrib/ivorysql_ora/sql/ora_sysview.sql` around lines 97 - 100, Extend the
test table definition used by the ALL_TAB_COLUMNS coverage by adding columns
that exercise numeric precision/scale and nullable cases: keep the existing id
(int4) and name (varchar) columns, then add a numeric/decimal column with
explicit precision and scale (e.g., NUMERIC(p,s)) to trigger precision/scale
handling, and add at least one column declared NULLABLE (no NOT NULL) to cover
the NULLABLE = 'Y' path; update any assertions in the test to expect the
precision/scale values and nullable flag for the new columns so ALL_TAB_COLUMNS
branches are exercised (refer to test_user_cons and the view logic that reads
column_precision/column_scale and nullable status).
| SELECT OWNER, TABLE_NAME, COLUMN_NAME, COLUMN_ID, | ||
| DATA_TYPE, DATA_LENGTH, DATA_PRECISION, DATA_SCALE, | ||
| NULLABLE | ||
| FROM ALL_TAB_COLUMNS | ||
| WHERE TABLE_NAME = 'test_user_cons'; |
There was a problem hiding this comment.
Scope the assertion to schema to avoid cross-schema flakiness.
At Line 106, filtering only by TABLE_NAME can return extra rows if another schema has the same table name.
Suggested fix
SELECT OWNER, TABLE_NAME, COLUMN_NAME, COLUMN_ID,
DATA_TYPE, DATA_LENGTH, DATA_PRECISION, DATA_SCALE,
NULLABLE
FROM ALL_TAB_COLUMNS
-WHERE TABLE_NAME = 'test_user_cons';
+WHERE TABLE_NAME = 'test_user_cons'
+ AND OWNER = current_schema();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SELECT OWNER, TABLE_NAME, COLUMN_NAME, COLUMN_ID, | |
| DATA_TYPE, DATA_LENGTH, DATA_PRECISION, DATA_SCALE, | |
| NULLABLE | |
| FROM ALL_TAB_COLUMNS | |
| WHERE TABLE_NAME = 'test_user_cons'; | |
| SELECT OWNER, TABLE_NAME, COLUMN_NAME, COLUMN_ID, | |
| DATA_TYPE, DATA_LENGTH, DATA_PRECISION, DATA_SCALE, | |
| NULLABLE | |
| FROM ALL_TAB_COLUMNS | |
| WHERE TABLE_NAME = 'test_user_cons' | |
| AND OWNER = current_schema(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contrib/ivorysql_ora/sql/ora_sysview.sql` around lines 102 - 106, The SELECT
against ALL_TAB_COLUMNS is only filtering by TABLE_NAME ('test_user_cons') which
can return rows from other schemas; update the WHERE clause in the query that
references ALL_TAB_COLUMNS to also restrict OWNER (e.g., OWNER = current_schema
or the expected schema name) so the assertion is scoped to the intended schema
and avoids cross-schema flakiness.
|
Thanks for your contribution. We will review it shortly. |
|
For issue 1009: |
Summary by CodeRabbit
New Features
Tests