refactor(schema): treat srid, dimensions and auto-increment as type parameters - #19
Merged
Merged
Conversation
…arameters These three were the entanglement left over from the previous commit. The modifiers were dropped by dialects that cannot emit them, but they could not simply be moved, because the library's own factories called them: Table::point() called srid(), Table::id() and Trait\Serial called autoIncrement(), and the three vector() factories called dimensions(). The block was framing. srid and dimensions are intrinsic to the column the way length is -- POINT SRID 3857, VECTOR(384) -- not modifiers applied afterwards, and the factories already receive them as arguments. So they move into Column::__construct() alongside length/precision/scale, threaded through newColumn(), with auto-increment as a flag beside them. That frees the modifiers to live only where the value is emitted: srid() MySQL, MariaDB, PostgreSQL dimensions() PostgreSQL autoIncrement() MySQL, MariaDB, PostgreSQL, SQLite The factories are untouched from a caller's perspective: point($name, $srid), id() and serial() work on every dialect and still record the values, so portable schema code is unaffected and every dialect emits byte-identical DDL. Table\ClickHouse::vector() and Table\MongoDB::vector() lose their $dimensions parameter. Array(Float64) and the array bsonType are unsized, so the argument could never be honoured -- two existing tests passed 768 and asserted output that ignored it, which is the clearest evidence it was never wired to anything. unsigned() stays on every dialect: it renders nothing on PostgreSQL and SQLite, but through an explicit overridable compileUnsigned() hook, which is a deliberate mapping rather than an oversight. It remains the single documented known gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📊 Coverage
Full per-file breakdown in the job summary. |
Contributor
Greptile SummaryThe PR moves SRID, vector dimensions, and auto-increment state into column construction while limiting their standalone modifiers to dialects that emit them.
Confidence Score: 4/5The PR should not merge until the base fluent-builder test is updated or the base Column autoIncrement() contract is restored. The changed base Column no longer exposes autoIncrement(), while an unchanged test directly invokes that method on a base Column and will fail with an undefined-method error. Files Needing Attention: src/Query/Schema/Column.php, tests/Query/Schema/FluentBuilderTest.php Important Files Changed
Prompt To Fix All With AI### Issue 1
src/Query/Schema/Column.php:73-82
**Base auto-increment method removed**
When `FluentBuilderTest::testColumnReturnsItselfForChainingFluentMethods` creates a base `Column`, the test still calls `autoIncrement()`, but this change removes that method from the base class, causing the test suite to terminate with an undefined-method error.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "refactor(schema): treat srid, dimensions..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #18 → #17 → #16 → #15. Closes the entanglement I flagged as out of scope there.
The blocker, and why it dissolved
These three modifiers were dropped by dialects that can't emit them, but I couldn't move them in #18 because the library's own factories called them:
Moving the modifier would have broken the factory on exactly the dialects that needed it.
The block turned out to be framing.
sridanddimensionsaren't modifiers at all — they're intrinsic to the column, the waylengthis.POINT SRID 3857,VECTOR(384): both are part of the column type, and the factories already take them as arguments. So they belong in the constructor next tolength/precision/scale, threaded throughnewColumn(), with the auto-increment flag beside them.Once the factories stop going through the public modifiers, the modifiers are free to live only where the value is emitted:
srid()POINT SRID 3857,GEOMETRY(POINT, 3857)dimensions()VECTOR(384)autoIncrement()AUTO_INCREMENT,IDENTITY,AUTOINCREMENTCallers are unaffected
point($name, $srid),id()andserial()still work on every dialect and still record the values — only the standalone modifiers are scoped. Verified byte-identical DDL against the pre-change output for all five dialects:A test pins both halves: the modifier is absent on ClickHouse/MongoDB/SQLite while
id()/serial()/point()still setisAutoIncrementandsridthere.One signature change
Table\ClickHouse::vector()andTable\MongoDB::vector()lose their$dimensionsparameter.Array(Float64)and the array bsonType are unsized, so it could never be honoured — and two existing tests passed768while asserting output that ignored it:That's the clearest evidence it was never wired to anything.
Table\PostgreSQL::vector($name, $dimensions)keeps it, because PostgreSQL emits it.Left as the one known gap
unsigned()stays on every dialect. It renders nothing on PostgreSQL and SQLite, but through an explicit overridablecompileUnsigned()hook returning''— a deliberate dialect mapping, not an oversight. It's the single remaining entry in the README's "Known gaps" note.Test plan
composer test— 5323 tests, 12383 assertions, all pass (was 5321)composer check— PHPStan level max, no errorscomposer lint— passid(),point(),serial(),vector()— identicalIntegration tests need Docker and were not run locally; CI covers them.
🤖 Generated with Claude Code