Preserve column-level OPTIONS in CREATE FOREIGN TABLE#6
Conversation
The columnDef path read ColId/Typename/ColQualList but ignored the sibling create_generic_options node, silently dropping column OPTIONS such as `bar INTEGER OPTIONS (column_name 'Bar') NOT NULL`. Both the river (aligned) and left-aligned code paths now emit the OPTIONS clause inline between the type and column constraints. Extracted a shared collect_generic_options helper so the inline and multi-line OPTIONS formatters reuse the same list-extraction logic. Fixes #5 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request adds support for formatting column-level Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/formatter/stmt.rs (2)
967-970: Guard against empty options in inline rendering.
format_col_generic_options_inlinewill emitOPTIONS ()if thecreate_generic_optionsnode has nogeneric_option_listchild (or an empty one). The callers already only invoke this whencreate_generic_optionsexists, but tree-sitter can produce a stub node on partial/ERROR input. Consider returning an empty string whenitemsis empty, mirroring the early-return informat_generic_options.♻️ Suggested change
fn format_col_generic_options_inline(&self, node: Node<'a>) -> String { let items = self.collect_generic_options(node); + if items.is_empty() { + return String::new(); + } format!("{} ({})", self.kw("OPTIONS"), items.join(", ")) }Callers would then also want to skip pushing an empty string into the parts list to avoid a stray extra space — or keep the push since
parts.join(" ")handles empties but yields a double-space. Simpler: have callers checkis_empty()before pushing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/formatter/stmt.rs` around lines 967 - 970, format_col_generic_options_inline can produce "OPTIONS ()" when collect_generic_options returns an empty vector; change format_col_generic_options_inline to return an empty string when items.is_empty() instead of formatting "OPTIONS ({})", and mirror the early-return behavior of format_generic_options. Update callers that push its return into parts (where parts.join(" ") is used) to skip pushing when the returned string is empty (or call .is_empty() before pushing) so no stray extra spaces are produced. Use the symbols format_col_generic_options_inline, collect_generic_options, format_generic_options, and the callers that push into parts to locate and apply the change.
414-416: Column OPTIONS now renders in regularCREATE TABLEdue to sharedclassify_table_elementlogic.
classify_table_elementis shared betweenformat_create_table_stmt(line 276) andformat_create_foreign_table_stmt(line 833), so regular table columns withOPTIONS(...)will now render the clause alongside other constraints. The alignment mechanism is correct—max_type_lendetermines column width, and trailingOPTIONS(...) NOT NULLflows into the constraints tail as intended.However, no fixture currently tests regular
CREATE TABLEwith column OPTIONS. The existing fixtures only cover foreign tables. Consider adding a test case to confirm the non-foreign output aligns properly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/formatter/stmt.rs` around lines 414 - 416, The shared classify_table_element logic causes regular CREATE TABLE columns with OPTIONS(...) to render the OPTIONS clause with other constraints (via format_col_generic_options_inline), so add a unit/fixture that covers a regular CREATE TABLE containing a column with OPTIONS(...) (not just foreign tables) to verify alignment and output; update or add a test similar to the existing foreign-table fixture but targeting format_create_table_stmt (and exercise classify_table_element and format_col_generic_options_inline) to assert the OPTIONS clause appears in the constraints tail and column alignment governed by max_type_len remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/formatter/stmt.rs`:
- Around line 967-970: format_col_generic_options_inline can produce "OPTIONS
()" when collect_generic_options returns an empty vector; change
format_col_generic_options_inline to return an empty string when
items.is_empty() instead of formatting "OPTIONS ({})", and mirror the
early-return behavior of format_generic_options. Update callers that push its
return into parts (where parts.join(" ") is used) to skip pushing when the
returned string is empty (or call .is_empty() before pushing) so no stray extra
spaces are produced. Use the symbols format_col_generic_options_inline,
collect_generic_options, format_generic_options, and the callers that push into
parts to locate and apply the change.
- Around line 414-416: The shared classify_table_element logic causes regular
CREATE TABLE columns with OPTIONS(...) to render the OPTIONS clause with other
constraints (via format_col_generic_options_inline), so add a unit/fixture that
covers a regular CREATE TABLE containing a column with OPTIONS(...) (not just
foreign tables) to verify alignment and output; update or add a test similar to
the existing foreign-table fixture but targeting format_create_table_stmt (and
exercise classify_table_element and format_col_generic_options_inline) to assert
the OPTIONS clause appears in the constraints tail and column alignment governed
by max_type_len remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19d75111-eda6-4ef6-89f9-2267d9720b9f
📒 Files selected for processing (3)
src/formatter/stmt.rstests/fixtures/river/create_foreign_table_column_options.expectedtests/fixtures/river/create_foreign_table_column_options.sql
|
Thanks @coderabbitai — evaluated both nitpicks and declining both on merit: 1. Empty-options guard in 2. Fixture for regular Both comments are unactionable; not making changes. |
|
Tip For best results, initiate chat on the files or code changes.
On point 1, this is consistent with the project's established error-tolerance policy (reject structural errors upstream, tolerate only sub-5-byte ERROR nodes), so defensive guarding here is indeed redundant. On point 2, that's a good clarification about the PostgreSQL grammar — ᓚᘏᗢ ✏️ Learnings added
🧠 Learnings used |
1 participant
Summary
CREATE FOREIGN TABLEsilently dropped column-levelOPTIONS (...)clauses — e.g.bar INTEGER OPTIONS (column_name 'Bar') NOT NULLlost the OPTIONS.columnDefformatter readColId,Typename, andColQualListbut ignored the siblingcreate_generic_optionsnode that tree-sitter produces.collect_generic_optionshelper so the inline and multi-line OPTIONS formatters share list-extraction logic.Fixes #5
Test plan
tests/fixtures/river/create_foreign_table_column_options.{sql,expected}covers the bug report.cargo test --test all_fixtures)cargo test)cargo clippy -- -D warningscleancargo fmt --checkcleanSummary by CodeRabbit
Release Notes
Bug Fixes
Tests