Repository navigation
Chg #960: Pass non-null and non-expression values only to dbTypecast() - #1197
KalimeroMK wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1197 +/- ##
=========================================
Coverage 98.62% 98.62%
- Complexity 1645 1649 +4
=========================================
Files 120 120
Lines 4288 4289 +1
=========================================
+ Hits 4229 4230 +1
Misses 59 59 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Drivers and custom code call ColumnInterface::dbTypecast() directly (e.g. with Expression default values in DDL builders, or Param values). Since Expression is Stringable, removing the ExpressionInterface arms silently stringified expressions instead of passing them through, breaking all driver test matrices. The wrapper function and internal call sites stay; the simplification of implementations needs a coordinated change across driver packages.
Expressions must reach ColumnInterface::dbTypecast(): some implementations process them (for example, MSSQL's binary column unwraps Param values into CONVERT expressions). Filtering them in the wrapper bypassed that processing and broke the MSSQL test matrix.
|
But it seems the function that only checks for |
|
Agreed that a null-only wrapper doesn't pay for itself — dropped the function and inlined the null check at the call sites in 82017d6. Benchmark is at parity with master now. |
|
I'd leave it as is and close the issue without changes. The PR found that |
|
Yes, I agree that in this case the repetition is unavoidable and is actually okay. |
|
@KalimeroMK thank you for the pull request. It helped. |
Fix #960
Changes
nullvalues are now passed through without callingColumnInterface::dbTypecast(). The check is inlined at the call sites inAbstractDMLQueryBuilder(insert/update/batch) andAbstractColumnDefinitionBuilder::buildDefault().No new API. The first version of this PR added a
dbTypecast()wrapper function, but a wrapper that only filtersnulladds call overhead for no measurable gain (see benchmark), so the check was inlined instead.DateTimeValueBuilderis untouched:DateTimeValue::$valueis non-nullable by type.Column implementations are intentionally left untouched. Two findings from the driver test matrices:
ExpressionisStringable, so removing theExpressionInterfacearms silently stringifies expressions instead of passing them through (breaks DDL builders in db-pgsql/db-mssql that passExpressiondefault values directly).Yiisoft\Db\Mssql\Column\BinaryColumn::dbTypecast()unwrapsParamvalues intoCONVERT(VARBINARY(MAX), ...)expressions, so expressions must keep reachingdbTypecast().Net effect: only
nullis safe to pre-filter. Simplifying the implementations needs a coordinated change across the driver packages and can be done as a follow-up for 3.0.Benchmark
2M calls per round, median of 7 rounds, PHP 8.5 with OPcache + JIT (tracing), workload: 80% scalars, 10%
null, 10% expressions,IntegerColumn+StringColumn:The inlined check is within noise of master; the wrapper function added ~4 ns per call on trivial casts.