Skip to content

Conversation

@ocean
Copy link
Owner

@ocean ocean commented Jan 17, 2026

Summary by CodeRabbit

  • Bug Fixes

    • Corrected handling of SQL statements that start with comments so command-type detection works reliably.
  • New Features

    • UPDATE and DELETE now include RETURNING results where applicable, returning values when available.
  • Documentation

    • Clarified that columns and rows may be nil for write operations (INSERT/UPDATE/DELETE).
  • Tests

    • Expanded tests to cover leading-comment scenarios and comprehensive RETURNING behavior for bulk updates/deletes.

✏️ Tip: You can customize this high-level summary in your review settings.

ocean added 4 commits January 17, 2026 08:55
Both STRICT tables and generated/computed columns are already fully implemented:

✅ el-z8u: STRICT Tables - create table(:users, strict: true) works
✅ el-ik6: Generated Columns - GENERATED ALWAYS AS syntax supported with STORED option

All tests pass. These features are production-ready.
Verified and marked as complete:

✅ el-4oc: R*Tree Spatial Indexing - create table(:locations, rtree: true) works
✅ el-o8r: Partial Index Support - CREATE INDEX with WHERE clause works
✅ el-0ez: RANDOM ROWID Support - create table(:sessions, random_rowid: true) works

All tests pass. Features are production-ready.
Fix Protocol.UndefinedError when queries start with SQL comments
(e.g., "-- comment\nSELECT ..."). Both single-line (--) and block
(/* */) comments are now correctly skipped before detecting the
query type.

Root cause: When a SELECT query started with a comment, both the
Rust should_use_query() and Elixir detect_command() functions
would not recognise it as a SELECT, causing the query to be
routed through the execute path which sets rows to nil. Ecto
then failed when trying to enumerate over nil rows.

Changes:
- Add skip_whitespace_and_comments() helper in Rust utils.rs
- Add skip_leading_comments_and_whitespace() helper in Elixir native.ex
- Update tests to verify comment handling works correctly
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 17, 2026

Walkthrough

Adds SQL comment-skipping to query-type detection in Elixir and Rust so statements beginning with -- or /* */ are classified correctly; documents :columns/:rows nil semantics for write operations; and appends generated RETURNING clause construction to the LibSQL adapter for update_all/delete_all.

Changes

Cohort / File(s) Summary
Elixir comment-skipping & command detection
lib/ecto_libsql/native.ex
Adds skip_leading_comments_and_whitespace/1 plus helpers (do_skip_comments/1, skip_to_newline/1, skip_to_block_end/1) and updates detect_command/1 to strip leading comments/whitespace before extracting the first token.
Elixir RETURNING support (adapter SQL construction)
lib/ecto/adapters/libsql/connection.ex
Appends a generated RETURNING clause to update_all/1 and delete_all/1; adds private helpers (returning/2, returning/3, returning_fields/3, returning_expr/3) and formatting logic to render fields while respecting SQLite restrictions (bare column names, no table aliases).
Elixir docs update
lib/ecto_libsql/result.ex
Documentation tweak: clarifies :columns and :rows may be nil for write operations without RETURNING.
Rust comment-skipping & utils
native/ecto_libsql/src/utils.rs
Introduces skip_whitespace_and_comments(bytes) -> usize to skip ASCII whitespace and -- / /* */ comments; replaces manual leading-skip in should_use_query; expands docs on string-literal limitations.
Rust tests
native/ecto_libsql/src/tests/utils_tests.rs
Expands test_sql_with_comments to assert correct detection when queries start with single-line or block comments (including mixed whitespace) and includes cases for INSERT with/without RETURNING.
Metadata
.beads/last-touched
Single-line deletion of a last-touched entry (non-functional).
New tests (Elixir)
test/returning_clause_test.exs
Adds comprehensive tests validating update_all and delete_all behaviors with and without select/RETURNING, including transactional scenarios and shape of returned rows (nil vs []).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐰 I hop past dashes, stars and a line,
Skipping the chatter so queries can shine.
I find the verb — UPDATE, SELECT, or DELETE —
Tidy the RETURNING, make results neat. 🥕✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title mentions 'nil return' but the PR primarily implements SQL comment skipping in query detection and adds RETURNING clause support for bulk operations, with only minor documentation updates about nil values. Consider a more descriptive title that accurately reflects the main changes, such as 'Add SQL comment handling to query detection and RETURNING clause support for bulk operations'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bugfix-queryable-execute-nil-return

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.

ocean added 2 commits January 18, 2026 16:16
When using update_all/delete_all with a select clause, Ecto expects
the returned rows instead of nil. This fix adds RETURNING clause
generation to these functions.

Changes:
- Add returning(query, sources) to update_all/1 and delete_all/1
- Add returning/2 function to extract fields from query select
- Add returning_fields/3 to format RETURNING clause fields
- Add returning_expr/3 to handle field expressions without table aliases

Fixes: Protocol.UndefinedError when calling update_all with select
Fixes: Oban job fetching which uses update_all with RETURNING
…te_all

Add test/returning_clause_test.exs with 12 tests covering:
- update_all with/without select clause
- delete_all with/without select clause
- Multiple fields in select
- Multiple rows affected
- Transactions with RETURNING
- Complex select expressions

These tests ensure the RETURNING clause fix works correctly across
various scenarios and prevents regression.
@ocean ocean merged commit cee6939 into main Jan 18, 2026
14 checks passed
@ocean ocean deleted the bugfix-queryable-execute-nil-return branch January 18, 2026 06:09
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.

2 participants