Revert "Merge pull request #627 from utopia-php/disable-array-index-m… #658
Conversation
WalkthroughEnable index array support by changing getSupportForIndexArray() to return true in both MySQL and MariaDB adapters. Removed outdated inline comments. No method signatures or other logic modified. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter (MySQL/MariaDB)
Client->>Database: createIndex(fields with array type)
Database->>Adapter (MySQL/MariaDB): getSupportForIndexArray()
Adapter (MySQL/MariaDB)-->>Database: true
Database->>Database: proceed with array index validation/creation
Database-->>Client: index created (arrays supported)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
abnegate
force-pushed
the
revert-arrayindex
branch
from
b6d92d6 to
cc10b50
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Database/Adapter/MariaDB.php (1)
2002-2005: Re‐enable MariaDB array indexes: CAST unsupported, fallback and composite‐index behavior must be verifiedBased on our inspection, MariaDB’s adapter currently:
- Inherits SQL::getSupportForCastIndexArray() (returns false) → the
CAST(... AS CHAR(N) ARRAY)path is never taken.- Overrides getSupportForIndexArray() to return true → array‐indexing will fall back to the non‐CAST path.
Please confirm the following before shipping:
• Fallback path for array indexes
– Ensure the code that generates a JSON/TEXT index applies a safe, fixed prefix length (using Database::ARRAY_INDEX_LENGTH) or uses generated columns. Without a length, indexing TEXT/JSON will fail in MariaDB.
– If no reliable fallback exists, override getSupportForIndexArray() in MariaDB to return false.• Composite indexes in shared-tables mode
– We unconditionally prepend_tenantto every index. Verify that MariaDB allows a composite functional (JSON/TEXT) index alongside a normal column. If it doesn’t, skip the tenant prefix for array‐index definitions.Suggested actions (pick one or both):
- Explicitly override getSupportForIndexArray() → return false in MariaDB until proper CAST or fallback logic is implemented.
- Enhance the MariaDB adapter’s fallback path to enforce prefix lengths or generated‐column indexing, and adjust shared-tables logic to omit
_tenantfor composite array indexes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Adapter/MariaDB.php(1 hunks)src/Database/Adapter/MySQL.php(1 hunks)
⏰ 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). (8)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (1)
src/Database/Adapter/MySQL.php (1)
81-87: MySQL array indexes re-enabled: ensure safe 8.0.x version and composite MVI supportPlease complete these checks before merging:
- Confirm your CI/production MySQL is ≥ 8.0.29 (ideally a recent GA such as 8.0.43+) so that the original “composite + MVI wrong results” bug (108640) and later regressions are all fixed.
- Composite indexes may include one multi-valued key part alongside other columns in any position—so prepending
_tenantis safe and does not need to be skipped for MVI indexes.- Verify no index definitions inadvertently include more than one MVI part, as only a single CAST(... ARRAY) key part is permitted per index.
…ysql"
This reverts commit 83278d6, reversing changes made to eb2f759.
Summary by CodeRabbit