Skip to content

NLS refactor#918

Merged
gaoxueyu merged 5 commits intoIvorySQL:IVORY_REL_5_STABLEfrom
OreoYang:nls_reconstruct
Oct 16, 2025
Merged

NLS refactor#918
gaoxueyu merged 5 commits intoIvorySQL:IVORY_REL_5_STABLEfrom
OreoYang:nls_reconstruct

Conversation

@OreoYang
Copy link
Copy Markdown
Collaborator

@OreoYang OreoYang commented Oct 13, 2025

  1. refactor NLS code by pg_bsd_indent
  2. fix make enable-git-hooks failed
  3. add auto reviews on IVORY_REL_5_STABLE branch

Summary by CodeRabbit

  • Chores

    • Added a minimal AI review configuration file.
    • Added a new code-format make target and removed the enable-git-hooks target.
    • Removed a deprecated configuration option from the Oracle compatibility extension.
  • Documentation

    • Updated English and Chinese READMEs to recommend make code-format.
  • Style

    • Whitespace and formatting cleanups across Oracle-compatible date/time and config modules; no behavior changes.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Oct 13, 2025

Walkthrough

Adds a CodeRabbit config, introduces a new phony make target code-format that runs the enable-git-hooks script (and removes the enable-git-hooks target from Makefile), updates README guidance, removes an NLS length-semantics GUC block, and applies widespread formatting/whitespace edits across Oracle datatype and GUC sources.

Changes

Cohort / File(s) Summary of Changes
Repo config
\.coderabbit.yaml
New config: schema link, language: en-US, reviews with profile: "chill", high_level_summaries: true, and auto_review enabled (drafts: false, base_branches: master, IVORY_REL_5_STABLE).
Build system targets
Make changes
GNUmakefile.in, Makefile
Added a new phony target code-format (depends on generated headers) which runs bash tools/enable-git-hooks.sh; removed the enable-git-hooks target from Makefile.
Docs (EN/CN)
README.md, README_CN.md
Updated developer setup instructions to recommend make code-format instead of make enable-git-hooks; direct script command remains bash tools/enable-git-hooks.sh.
IvorySQL Ora GUCs
contrib/ivorysql_ora/src/guc/guc.c
Removed previously-#if-0 guarded NLS length-semantics scaffolding: static enum/options and the DefineCustomEnumVariable invocation were deleted.
Oracle datatype sources — formatting
contrib/ivorysql_ora/src/datatype/oradate.c, .../oratimestamp.c, .../oratimestampltz.c, .../oratimestamptz.c
Formatting, whitespace, comment and minor punctuation edits only; no logic, control-flow, or API changes.
Oracle SQL comments
contrib/ivorysql_ora/sql/ora_datetime.sql
Comment reflow/spacing changes (presentational only).
Datetime builtin functions — formatting
contrib/ivorysql_ora/src/builtin_functions/datetime_datatype_functions.c
Widespread whitespace/indentation changes and reflow; no semantic or API changes.
Core GUC tables / IVY GUC impl — formatting
src/backend/utils/misc/guc_tables.c, src/backend/utils/misc/ivy_guc.c
Re-indentation, comment wrapping, minor message formatting; behavior and public signatures unchanged.
Makefile docs/test targets
GNUmakefile.in (additional)
Added code-format to .PHONY list.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Dev as Developer
    participant Make as Make/GNUmakefile.in
    participant Script as tools/enable-git-hooks.sh

    Dev->>Make: run `make code-format`
    Note right of Make #dff0d8: new phony target added
    Make->>Script: exec `bash tools/enable-git-hooks.sh`
    Script-->>Make: installs/enables hooks (sync)
    Make-->>Dev: return status
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • bigplaice

Poem

I twitch my nose at tidy code tonight,
A new code-format hop brings hooks to light.
Whitespace trimmed, old GUCs tuck away,
Docs updated for the developer's day.
I nibble bytes and bound out of sight — hooray! 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 49.09% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly identifies the primary purpose of the changeset as a refactoring of the National Language Support (NLS) code, which represents the most extensive and significant change in the pull request. It is concise and specific, avoiding vague or generic terminology while accurately reflecting the main developer intent. Although the PR also fixes the git-hooks target and adds review automation, those are secondary to the core NLS refactor and need not be captured in the title. Consequently, the title succinctly conveys the biggest change without unnecessary detail.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e371807 and 17428ae.

⛔ Files ignored due to path filters (3)
  • contrib/ivorysql_ora/expected/ora_datetime.out is excluded by !**/*.out
  • contrib/ivorysql_ora/expected/ora_datetime_datatype_functions.out is excluded by !**/*.out
  • src/oracle_test/regress/expected/expressions.out is excluded by !**/*.out
📒 Files selected for processing (2)
  • contrib/ivorysql_ora/sql/ora_datetime.sql (2 hunks)
  • contrib/ivorysql_ora/src/builtin_functions/datetime_datatype_functions.c (72 hunks)
✅ Files skipped from review due to trivial changes (2)
  • contrib/ivorysql_ora/sql/ora_datetime.sql
  • contrib/ivorysql_ora/src/builtin_functions/datetime_datatype_functions.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: meson_build (ubuntu-latest)
  • GitHub Check: contrib_regression
  • GitHub Check: oracle_regression (ubuntu-latest)
  • GitHub Check: oracle_pg_regression (ubuntu-latest)
  • GitHub Check: pg_regression (ubuntu-latest)
  • GitHub Check: build (ubuntu-latest)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e7d0b3a and 0484347.

📒 Files selected for processing (12)
  • .coderabbit.yaml (1 hunks)
  • GNUmakefile.in (1 hunks)
  • Makefile (0 hunks)
  • README.md (1 hunks)
  • README_CN.md (1 hunks)
  • contrib/ivorysql_ora/src/datatype/oradate.c (4 hunks)
  • contrib/ivorysql_ora/src/datatype/oratimestamp.c (25 hunks)
  • contrib/ivorysql_ora/src/datatype/oratimestampltz.c (6 hunks)
  • contrib/ivorysql_ora/src/datatype/oratimestamptz.c (7 hunks)
  • contrib/ivorysql_ora/src/guc/guc.c (2 hunks)
  • src/backend/utils/misc/guc_tables.c (7 hunks)
  • src/backend/utils/misc/ivy_guc.c (15 hunks)
💤 Files with no reviewable changes (1)
  • Makefile
🧰 Additional context used
🧬 Code graph analysis (5)
contrib/ivorysql_ora/src/datatype/oratimestamptz.c (5)
src/backend/utils/adt/timestamp.c (1)
  • timestamptz_in (417-481)
src/include/postgres.h (4)
  • CStringGetDatum (354-358)
  • ObjectIdGetDatum (256-260)
  • Int32GetDatum (216-220)
  • PointerGetDatum (326-330)
src/backend/utils/adt/formatting.c (1)
  • timestamptz_to_char (4667-4712)
src/include/utils/timestamp.h (1)
  • TimestampTzGetDatum (51-55)
src/backend/utils/adt/varlena.c (1)
  • cstring_to_text (193-197)
contrib/ivorysql_ora/src/datatype/oradate.c (4)
src/backend/utils/adt/date.c (2)
  • date_timestamp (1347-1356)
  • date_in (113-179)
src/include/postgres.h (2)
  • CStringGetDatum (354-358)
  • PointerGetDatum (326-330)
src/include/utils/timestamp.h (1)
  • TimestampGetDatum (45-49)
src/backend/utils/adt/varlena.c (1)
  • cstring_to_text (193-197)
contrib/ivorysql_ora/src/datatype/oratimestamp.c (4)
src/backend/utils/adt/timestamp.c (1)
  • timestamp_in (165-228)
src/include/postgres.h (4)
  • CStringGetDatum (354-358)
  • ObjectIdGetDatum (256-260)
  • Int32GetDatum (216-220)
  • PointerGetDatum (326-330)
src/include/utils/timestamp.h (2)
  • TimestampGetDatum (45-49)
  • IntervalPGetDatum (57-61)
src/backend/utils/adt/varlena.c (1)
  • cstring_to_text (193-197)
contrib/ivorysql_ora/src/datatype/oratimestampltz.c (5)
src/backend/utils/error/elog.c (2)
  • errmsg (1070-1086)
  • errcode (853-864)
src/backend/utils/adt/formatting.c (1)
  • timestamptz_to_char (4667-4712)
src/include/utils/timestamp.h (1)
  • TimestampTzGetDatum (51-55)
src/include/postgres.h (1)
  • PointerGetDatum (326-330)
src/backend/utils/adt/varlena.c (1)
  • cstring_to_text (193-197)
src/backend/utils/misc/ivy_guc.c (1)
src/backend/utils/error/elog.c (3)
  • errcode (853-864)
  • errmsg (1070-1086)
  • errhint (1317-1332)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: oracle_regression (ubuntu-latest)
  • GitHub Check: build (ubuntu-latest)
  • GitHub Check: meson_build (ubuntu-latest)
  • GitHub Check: oracle_pg_regression (ubuntu-latest)
  • GitHub Check: contrib_regression
  • GitHub Check: pg_regression (ubuntu-latest)
🔇 Additional comments (16)
contrib/ivorysql_ora/src/datatype/oradate.c (1)

1-746: LGTM! Formatting changes only.

The changes in this file are purely cosmetic whitespace and alignment adjustments. No functional changes detected.

README.md (1)

35-35: LGTM! Documentation accurately reflects the new workflow.

The update from make enable-git-hooks to make code-format correctly documents the new build target introduced in GNUmakefile.in while preserving the alternative command path.

contrib/ivorysql_ora/src/datatype/oratimestamp.c (1)

1-1705: LGTM! Formatting changes only.

All changes are cosmetic whitespace and alignment adjustments with no functional impact.

GNUmakefile.in (1)

161-167: LGTM! Build target properly implemented.

The new code-format target is correctly integrated:

  • Depends on submake-generated-headers (appropriate for formatting tools that need generated headers)
  • Properly declared in .PHONY
  • Aligns with the documentation updates in README.md and README_CN.md
README_CN.md (1)

21-21: LGTM! Chinese documentation matches English version.

The Chinese README correctly documents the new make code-format workflow, maintaining consistency with README.md.

contrib/ivorysql_ora/src/datatype/oratimestamptz.c (1)

1-757: LGTM! Formatting changes only.

All changes are cosmetic whitespace and alignment adjustments with no functional impact.

.coderabbit.yaml (1)

1-10: LGTM! Minimal and appropriate CodeRabbit configuration.

The configuration is well-structured:

  • Schema validation enabled for editor support
  • chill profile appropriate for less strict reviews
  • Auto-review enabled for the specified base branches (master and IVORY_REL_5_STABLE)
  • Drafts excluded from auto-review (line 9), which is a good practice
contrib/ivorysql_ora/src/guc/guc.c (1)

39-43: LGTM! Dead code removal.

The removal of the disabled NLS length semantics GUC code is appropriate:

  • The code was already wrapped in #if 0 blocks (disabled)
  • Removes unused internal scaffolding (nls_length_semantics_options array and nls_length_semantics global)
  • IvorysqlOraDefineGucs is now an empty function, which is valid for future GUC additions
contrib/ivorysql_ora/src/datatype/oratimestampltz.c (2)

200-212: LGTM!

The formatting changes to variable declarations improve code readability and are consistent with the codebase style.


239-245: LGTM!

Function call formatting is cleaner and more readable.

src/backend/utils/misc/ivy_guc.c (3)

33-54: LGTM!

Variable declaration formatting is consistent and improves code readability.


449-493: LGTM!

The formatting changes to these check functions improve readability while preserving all logic and error handling.


103-103: LGTM!

All formatting changes throughout the file are consistent with the pg_bsd_indent refactoring objective and improve code layout without altering functionality.

Also applies to: 240-240, 288-310

src/backend/utils/misc/guc_tables.c (3)

2154-2156: LGTM!

The formatting of IVY_GUC macro include blocks is consistent and properly paired. The blank lines improve code readability.

Also applies to: 3892-3894, 4177-4179, 4990-4992, 5450-5452


579-579: LGTM!

Variable declaration formatting is consistent with surrounding code.


5134-5135: LGTM!

Comment formatting change improves consistency.

Comment thread contrib/ivorysql_ora/src/datatype/oratimestampltz.c Outdated
Comment thread contrib/ivorysql_ora/src/datatype/oratimestampltz.c Outdated
@OreoYang
Copy link
Copy Markdown
Collaborator Author

@coderabbitai generate docstrings

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Oct 13, 2025

✅ Actions performed

Initiated docstring generation; will generate only if new commits exist.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Oct 13, 2025

Note

Docstrings generation - SUCCESS
Generated docstrings for this pull request at #919

coderabbitai bot added a commit that referenced this pull request Oct 13, 2025
Docstrings generation was requested by @OreoYang.

* #918 (comment)

The following files were modified:

* `contrib/ivorysql_ora/src/datatype/oradate.c`
* `contrib/ivorysql_ora/src/datatype/oratimestamp.c`
* `contrib/ivorysql_ora/src/datatype/oratimestampltz.c`
* `contrib/ivorysql_ora/src/datatype/oratimestamptz.c`
* `contrib/ivorysql_ora/src/guc/guc.c`
* `src/backend/utils/misc/ivy_guc.c`
int last_day = 0;
Timestamp result;

int y = 0,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatted result looks worse than before. How do you think?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is auto generated by tools/pg_bsd_indent, which is following pg code style,

@bigplaice
Copy link
Copy Markdown
Collaborator

No comments, proceed.

@gaoxueyu gaoxueyu merged commit 89e4b15 into IvorySQL:IVORY_REL_5_STABLE Oct 16, 2025
6 checks passed
@OreoYang OreoYang mentioned this pull request Oct 16, 2025
@OreoYang OreoYang deleted the nls_reconstruct branch November 14, 2025 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants