Support OBJECT data type in tablet write - #175
Conversation
Add support for the OBJECT data type (TSDataType 12) in the table model tablet write path, following the Java TableSession/tsfile implementation: - Add OBJECT to the TSDataType enum and the string/byte type maps - Treat OBJECT as a binary column (like BLOB) in Tablet: value storage, SetValueAt/GetValueAt, Swap, getValuesBytes and NewTablet - Add Tablet.SetObjectValueAt for segmented OBJECT writes, wrapping each segment with a 1-byte isEOF flag and an 8-byte big-endian offset, matching Java Tablet.addValue(rowIndex, columnIndex, isEOF, offset, content) - Decode OBJECT (and BLOB/STRING) binary columns in the read path so written objects can be read back (e.g. via READ_OBJECT) - Add unit tests for whole-object and segmented OBJECT tablet writes - Add an e2e table test writing OBJECT via tablet, covering whole-object, segmented and null-object rows Also include the regenerated thrift common code (new aggregation types, pipeRecentFailureList field) from `make all`.
There was a problem hiding this comment.
Pull request overview
Adds OBJECT tablet write/read support, including segmented object uploads and protocol decoding.
Changes:
- Adds OBJECT type handling across tablet serialization and query decoding.
- Introduces segmented OBJECT writes with tests.
- Regenerates thrift common types and fields.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
client/protocol.go |
Defines OBJECT type mappings. |
client/tablet.go |
Adds OBJECT storage, serialization, and segmented writes. |
client/tablet_test.go |
Tests OBJECT tablet behavior. |
client/column_decoder.go |
Decodes binary OBJECT columns. |
client/rpcdataset.go |
Supports OBJECT result getters. |
test/e2e/e2e_table_test.go |
Adds OBJECT write/read integration coverage. |
common/common.go |
Updates generated thrift definitions. |
Files not reviewed (1)
- common/common.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| _src38 := _src37[k] | ||
| if _tgt != _src38 { return false } |
f08f709 to
c3bf6b4
Compare
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
The segmented OBJECT write envelope matches the Java implementation and passed live round-trip testing against TimechoDB. However, the public OBJECT read/write semantics and null-bitmap handling still have reproducible correctness issues, and the unrelated generated Thrift changes should be removed from this PR. Please address the inline comments before merging.
| return binary.GetStringValue(), nil | ||
| } | ||
| case BLOB: | ||
| case BLOB, OBJECT: |
There was a problem hiding this comment.
[P1] Decode OBJECT results with OBJECT semantics
Grouping OBJECT with BLOB exposes server-side OBJECT metadata instead of the public object representation. In a live TimechoDB test, SELECT file returned an OBJECT payload whose first eight bytes are the object size and whose remaining bytes contain the internal object path; this branch makes GetObject return those raw bytes, while the matching GetString branch hex-encodes them. The Java client uses BytesUtils.parseObjectByteArrayToString(...) for both getters (for example, (Object) 1.00 KB) and rejects getBlob for OBJECT. Please split OBJECT from BLOB, implement the same formatter/getter contract, add direct SELECT file tests, and add the corresponding database/sql column mapping so a non-null OBJECT is not silently returned as nil.
| return fmt.Errorf("illegal argument value %v %v", value, reflect.TypeOf(value)) | ||
| } | ||
| case TEXT, STRING: | ||
| case TEXT, STRING, OBJECT: |
There was a problem hiding this comment.
[P1] Do not accept unframed OBJECT payloads here
For TableSession OBJECT writes, the server interprets every value as a segment envelope: one EOF byte, an eight-byte big-endian offset, then content. This branch accepts an arbitrary string or []byte and serializes it without that envelope; a live insertion of a string accepted here failed with status 741 because the server interpreted the string bytes as the offset. Please remove OBJECT from this generic branch and have SetObjectValueAt assign through an internal framed-value helper, or expose a separate, clearly documented API if another valid pre-encoded/object-path representation is required. The unit test should not assert that raw "hello" is a valid TableSession OBJECT value.
| } | ||
| binary.BigEndian.PutUint64(value[1:9], uint64(offset)) | ||
| copy(value[9:], content) | ||
| return t.SetValueAt(value, columnIndex, rowIndex) |
There was a problem hiding this comment.
[P2] Clear a previously marked NULL bit before storing the segment
SetValueAt(nil, ...) marks this cell in the bitmap, but the non-nil path never unmarks it. Consequently, calling SetValueAt(nil, ...) and then SetObjectValueAt(...) for the same cell still writes a NULL row; this was reproducible against TimechoDB. The equivalent Java overload calls updateBitMap(rowIndex, columnIndex, false). Please clear the bit for non-nil assignments (ideally in the shared SetValueAt path) and add a regression test for overwriting NULL with an OBJECT value.
| TAggregationType_SKEWNESS TAggregationType = 38 | ||
| TAggregationType_KURTOSIS TAggregationType = 39 | ||
| TAggregationType_PERCENTILE TAggregationType = 40 | ||
| TAggregationType_RATE TAggregationType = 41 |
There was a problem hiding this comment.
[P2] Keep unrelated generated protocol updates out of this OBJECT PR
These aggregation constants and the pipeRecentFailureList additions are generated from a newer IoTDB protocol and are unrelated to OBJECT tablet support. Bundling them expands the public Thrift/protocol surface, creates version drift, and also brings in the correctness issue already reported in the Equals implementation below. Please revert common/common.go from this PR and submit any pinned, reviewed IDL regeneration as a separate change.
Description
Adds support for the OBJECT data type (TSDataType
12) in the table-model tablet write path of the Go client, following the JavaTableSession/ tsfile implementation.Write path
OBJECTto theTSDataTypeenum and the string/byte type maps (client/protocol.go)Tablet(client/tablet.go): value storage,SetValueAt/GetValueAt,Swap,getValuesBytes,NewTabletTablet.SetObjectValueAt(isEOF, offset, content, columnIndex, rowIndex)for segmented OBJECT writes: each segment is wrapped with a 1-byteisEOFflag + 8-byte big-endian offset, matching JavaTablet.addValue(rowIndex, columnIndex, isEOF, offset, content)Read path
BinaryArrayColumnDecoder, and handle OBJECT like BLOB inIoTDBRpcDataSetgetters, so written objects can be read back (e.g.select READ_OBJECT(file)+GetBlob)Tests
TestTablet_OBJECT(whole-object write),TestTablet_SetObjectValueAt(segmented write) inclient/tablet_test.goTest_InsertObjectTabletintest/e2e/e2e_table_test.go, covering whole-object, segmented (512B), and null-object rows, verified viaREAD_OBJECT/GetBlob,count(*), null check, and tag/field round-tripAlso includes the regenerated thrift
commoncode (new aggregation types,pipeRecentFailureListfield) produced bymake all.Verification
go build ./.../go test ./client/...passmake e2e_test)