fix(#10137): INSERT INTO a TIMESERIES type no longer coerces a number into a BOOLEAN or STRING column - #10236

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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 help to get the list of available commands.}

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

Review

Targeted fix with a good parity-table test. The change is small and the rationale is well documented. A few points:

Potential issues

1. Skipped conversion applies to every MutableDocument of a TIMESERIES type, not only the INSERT path. convertValueToSchemaType is also reached by UPDATE/set and by documents materialized from query results. If a TIMESERIES doc is ever mutated and read back before the append, it can now hold an unconverted value (e.g. Integer in a LONG column, or "5" in an INTEGER column) in memory. Please confirm nothing between set and storableValue relies on the declared class (property validation such as min/max/mandatory/regexp runs on the raw value now; a "5" string against max could behave differently or throw a ClassCastException). A test with a constrained TIMESERIES property would pin this.
2. Behavior change for SQL users: bo = 1 and s = 42 now fail where they used to work. This is the stated intent, but it is a breaking change. Worth a release-note line, and the Issue9809 test name (aBooleanColumnIsConvertedBeforeTheStorageLayerWhateverTheWriter) is now misleading, since SQL no longer converts first. Consider renaming.
3. Error type for out-of-range numbers changed (IllegalArgumentException to CommandExecutionException). Check other modules (server HTTP error mapping, Studio, gRPC/Bolt tests) for assertions on the old message or status code.

Code quality

• isConvertedOnAppendOnly is a confusing name (reads as "only converted on append"). isConvertedByStorableValueOnly or skipsDocumentConversion would be clearer.
• The lookup is a linear scan over columns per set; fine for small column lists, and only runs for TIMESERIES types. OK per the performance guidance.
• In the test, viaSql swallows every RuntimeException, so an unrelated failure (e.g. a typo in SQL) counts as REFUSED and can match a gateway refusal. Consider asserting the exception is a CommandExecutionException (or the pinned type) there.
• Pair/REFUSED helpers are fine; consider @Tag-free is OK since the test is fast (40 type creations).

Tests

• Good coverage of SET/CONTENT/VALUES. Missing: UPDATE on a TIMESERIES type and a constrained-property case (see point 1), and a valid-value round trip for each non-temporal type via SQL (string "5" into INTEGER is covered, Float/Short storage are covered by the table).

No security concerns. Overall LGTM pending point 1.

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review cycle 1, addressed in 6331217:

• Point 1 (unconverted values in a TIMESERIES document): checked, not reachable. No write path to a TIMESERIES type validates the document: SaveElementStep routes the row to saveToTimeSeries before any validate(), and FetchFromTimeSeriesStep reads rows into ResultInternal, never into a MutableDocument. UPDATE on a TIMESERIES type matches no records. A probe also showed that ALTER PROPERTY M.i MAX 10 and REGEXP are accepted and then ignored before and after this PR (i = 50 is stored). That is a separate defect, filed as TIMESERIES: ALTER PROPERTY accepts MAX/MIN/REGEXP/MANDATORY on a column, and no write path ever enforces them #10238.
• Point 2: the behavior change is now listed under Residual risk in the PR body as a release-note item. Renamed the A datetime written to a TIMESERIES DATETIME column through the line protocol still fails with a raw ClassCastException, because only the SQL INSERT path converts the value #9809 test to aBooleanColumnRefusesANumberOnEverySurfaceAndTheStorageLayerNamesTheColumn.
• Point 3: grepped server, grpcw, grpc-client, bolt, postgresw, integration and network tests. None assert on the old Cannot convert type ... found in property message for a TIMESERIES insert.
• Naming: isConvertedOnAppendOnly is now skipsDocumentConversion.
• viaSql: now catches only CommandExecutionException, so any other exception fails the test instead of matching a gateway refusal.

Review of #10236

Overall: a small, well-targeted fix with a strong table-driven parity test. Skipping the document-layer Type.convert for non-temporal TAG/FIELD columns so storableValue is the single conversion is sound, and the PR description is clear about scope and known gaps.

Correctness / behaviour

1. The early return in MutableDocument.convertValueToSchemaType also skips Type.convert(..., property). TIMESERIES columns are scalar so this is likely fine, but please confirm storableValue covers every column type the DDL accepts (e.g. DECIMAL).
2. The in-memory document now holds the raw value (e.g. Integer 42 for a STRING property) until SaveElementStep. Anything between set and save that reads by schema type (property constraints like regexp/min/max, validate(), RETURN AFTER) could now see an unconverted value, e.g. a ClassCastException instead of a clean error. Please confirm or add a test with a constrained column.
3. convertValueToSchemaType is shared by every set, not just INSERT. If UPDATE on a TIMESERIES type is not blocked, that path changes too.
4. The behaviour change (INSERT ... SET bo = 1 now fails) is intentional and documented, but breaking for SQL users who write 1/0 for booleans; worth a release note.

Performance
Cost is an instanceof plus a linear getTsColumn scan per set on TIMESERIES types only. Fine.

Tests

• Good: red-before/green-after, SQL vs gateway parity, SET/CONTENT/VALUES covered, and the old test that asserted the bug was updated.
• Missing: a TAG column case (all parity pairs use FIELDS, though the fix explicitly covers TAG), and a multi-row INSERT ... VALUES (...), (...).
• Consider asserting the refusal message matches the gateway's for a few pairs, not only refused/refused.

Security
No concerns; validation becomes stricter, not looser.

Follow-ups #10232 and #10233 are properly tracked. The items to check before merge are the constrained-property validation, the UPDATE path, and TAG coverage.

Review cycle 2, addressed in de954a1:

• 1 (DECIMAL and other types): TimeSeriesTypeBuilder.validate refuses every column type except BOOLEAN, BYTE, SHORT, INTEGER, LONG, FLOAT, DOUBLE, STRING and the DATE/DATETIME family (SchemaException: Column 'd' of type DECIMAL cannot be used in a TIMESERIES type). Each non-temporal one has its own rule in storableValue. The javadoc of skipsDocumentConversion now says so.
• 2 and 3 (constraints, validate(), UPDATE): answered in the cycle-1 reply. Nothing validates a TIMESERIES document on any path, UPDATE matches no records, and the ignored constraints are pre-existing, filed as TIMESERIES: ALTER PROPERTY accepts MAX/MIN/REGEXP/MANDATORY on a column, and no write path ever enforces them #10238. RETURN on an INSERT into a TIMESERIES type reports the row the statement built, so it now shows the value as written.
• 4: release-note line is in the PR body (Residual risk).
• Tests: the parity table now runs BOOLEAN/STRING/INTEGER/LONG pairs as a TAG too. The line protocol carries a tag only as text, so those rows are held to storableValue appended through the engine. Where both sides refuse, the table now also requires the gateway's reported reason to end with the statement's message. Added aMultiRowValuesIsRefusedAtTheRowWithANumberForABooleanColumn.

Review of #10236

Solid, narrowly scoped fix. The root cause is correctly identified (Type.convert runs before storableValue), the fix sits at the single choke point so SET, CONTENT and VALUES are all covered, and the PR description clearly lists what is deferred (#10232, #10233).

Code quality

• MutableDocument.convertValueToSchemaType: the instanceof guard is cheap and the comment explains the why.
• LocalTimeSeriesType.skipsDocumentConversion: Javadoc is accurate. Please confirm getTsColumn(name) is a map lookup and not a linear scan, since it now runs on every set() of a declared property of a timeseries type.
• The early return also bypasses anything else the method does after the type conversion (e.g. constraints, if applied there). Timeseries columns likely cannot carry those, but worth confirming or noting in the Javadoc.

Potential issues

• The in-memory MutableDocument now holds the raw value (e.g. an Integer for a BYTE column) instead of the declared class. Anything reading it back between set() and SaveElementStep (e.g. INSERT ... RETURN, triggers) sees the unconverted value. A small test for INSERT ... RETURN on a timeseries type would pin the behavior.
• The behavior change (number into BOOLEAN/STRING now refused) is intended, but should be mentioned in release notes/docs since it can break existing client inserts.

Tests

• The 40-pair parity table is a good guarantee-style test. Minor: sameOutcome has redundant a/b aliases, and the endsWith reason match is lenient.
• aMultiRowValuesIsRefused... asserts <= 1 rows; if semantics are deterministic, asserting exactly 1 is a stronger regression guard.
• Consider explicit TAG-column coverage through CONTENT/VALUES (the table covers tags via SET only).

Performance / security
No concerns: one instanceof plus a column lookup for timeseries types only; validation becomes stricter, not looser.

LGTM with the small confirmations above.

Review cycle 3, addressed in 75ffd58:

• Redundant aliases in sameOutcome: removed.
• getTsColumn linear scan: confirmed, it is the existing indexed loop over the column list (a handful of entries, no iterator allocation; its javadoc explains the choice). It runs only for TIMESERIES types. Not changed.
• Anything after the conversion: the early return sits right before Type.convert, the last thing convertValueToSchemaType does. Constraints are never applied on a TIMESERIES write (TIMESERIES: ALTER PROPERTY accepts MAX/MIN/REGEXP/MANDATORY on a column, and no write path ever enforces them #10238).
• Not changed, with rationale: (a) an INSERT ... RETURN test would pin the in-memory row as written ('5' for an INTEGER column whose stored sample is 5). I'd rather not freeze that shape, since TIMESERIES INSERT has no RETURN contract today. (b) The multi-row test keeps <= 1, the same bound Issue10064...aBatchInsertRefusesTheRowWithAValueTheColumnCannotHold uses, because a time series append commits on its own and the row count before the refusal is not part of the statement's contract. (c) TAG through CONTENT/VALUES goes through the same MutableDocument.putProperty choke point that SET and the field CONTENT/VALUES tests already exercise.

Review

Small, well-targeted fix. Hooking convertValueToSchemaType covers SET, CONTENT and VALUES at once, and the parity-table test is a strong guarantee. I read the diff only and did not run the tests.

Good

• Minimal change; the cost is one instanceof per set on a declared property.
• skipsDocumentConversion is documented, and the temporal carve-out points to the follow-up issue.
• Updating the old test that asserted the bug is the right call.
• Coverage of SET, CONTENT, VALUES, multi-row VALUES and BYTE 300.

Suggestions

1. Lookup cost: getTsColumn(name) runs on every set() for TIMESERIES types. If it is a linear scan or allocates, consider a precomputed structure (performance-first rule).
2. Values left unconverted: the document now keeps raw values (e.g. a String in an INTEGER column) until saveToTimeSeries. Anything between set() and the save that reads the property by declared type (validation, toJSON, RETURN AFTER) sees the raw value. A test reading the returned record after a successful insert would pin this down.
3. Null handling: add a null pair for a non-temporal column; SQL and gateway should agree.
4. Behavior change: SQL INSERT of 1 into BOOLEAN or 42 into STRING now fails. Call it out in release notes/docs as breaking.
5. Test style: sameOutcome/viaSql rely on try/catch and message matching, so failures will be harder to read. Minor.
6. Test runtime: 40+ pairs each create a type; if the class takes multiple seconds, tag it @Tag("slow").

Security / correctness
No security concerns. Nothing is stored on refusal, as the tests assert. Known gaps (10232, 10233) are tracked.

Items 1 and 2 are the ones I would check before merge.

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.46%. Comparing base (ffc4de1) to head (75ffd58).
⚠️ Report is 2 commits behind head on main.

@@             Coverage Diff              @@
##               main   #10236      +/-   ##
============================================
+ Coverage     78.43%   78.46%   +0.02%     
+ Complexity     4651     4650       -1     
============================================
  Files          2252     2252              
  Lines        238403   238484      +81     
  Branches      51602    51615      +13     
============================================
+ Hits         186993   187115     +122     
+ Misses        32055    32021      -34     
+ Partials      19355    19348       -7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

I checked the reproduction from #10137 against this PR's head, 75ffd58, and against main at a2c54ef. Both are the CI-built distribution jars, run as an embedded database on JDK 21 and on JDK 25 (with compact object headers), with identical results on both JDKs.

The reproduction is the issue's schema, CREATE TIMESERIES TYPE M TIMESTAMP ts FIELDS (i INTEGER, d DOUBLE, s STRING, bo BOOLEAN, b BYTE, l LONG) SHARDS 1, followed by one INSERT INTO M per row below.

Sibling forms the description does not list, same outcome on both JDKs:

I did not run DATE or DATETIME columns, and I did not run the line protocol, which this PR does not touch.

Seen on both builds and outside this PR's scope: a STRING field the INSERT does not set reads back as "", where every other unset field of the row reads null (e.g. after SET l=5: {"i":null,"d":null,"s":"","bo":null,"b":null,"l":5}).

@tae898 what are your conclusions? Is this PR ok? May I merge it?

Yes, it looks good to merge from my side. On the head I tested (75ffd580be, still the head), a number into a BOOLEAN or STRING field is refused for SET, VALUES and CONTENT alike, out-of-range values now raise a CommandExecutionException instead of a bare IllegalArgumentException, and the conversions that should still work (d='2.5', bo='TRUE', l=5) are unchanged. Not covered by my check: DATE/DATETIME fields, the line protocol, and the unset STRING reading "", which happens on main too and is outside this PR.