JackieTien97 commented on code in PR #175:
URL: https://github.com/apache/iotdb-client-go/pull/175#discussion_r3820930145
##########
client/tablet.go:
##########
@@ -195,7 +195,7 @@ func (t *Tablet) SetValueAt(value interface{}, columnIndex,
rowIndex int) error
default:
return fmt.Errorf("illegal argument value %v %v",
value, reflect.TypeOf(value))
}
- case TEXT, STRING:
+ case TEXT, STRING, OBJECT:
Review 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.
##########
client/tablet.go:
##########
@@ -229,6 +229,42 @@ func (t *Tablet) SetValueAt(value interface{},
columnIndex, rowIndex int) error
return nil
}
+// SetObjectValueAt writes a segment of an OBJECT column value. An OBJECT
value can be
+// written in multiple segments so that a large object does not need to be
fully loaded
+// into memory: each segment is wrapped into a 9-byte header (1 byte isEOF
flag followed
+// by an 8-byte big-endian offset) and then the raw content, consistent with
the Java
+// Tablet.addValue(rowIndex, columnIndex, isEOF, offset, content). Segments of
the same
+// object must be written in order with ascending offsets, and the last
segment must set
+// isEOF to true.
+//
+// Parameters:
+// - isEOF: Whether this segment is the last one of the object.
+// - offset: The offset of this segment within the whole object.
+// - content: The raw bytes of this segment.
+// - columnIndex: The column index of the OBJECT column.
+// - rowIndex: The row index to write the segment into.
+//
+// Returns:
+// - err: An error if the column/row index is invalid or the column is not
of type OBJECT.
+func (t *Tablet) SetObjectValueAt(isEOF bool, offset int64, content []byte,
columnIndex, rowIndex int) error {
+ if columnIndex < 0 || columnIndex >= len(t.measurementSchemas) {
+ return fmt.Errorf("illegal argument columnIndex %d",
columnIndex)
+ }
+ if rowIndex < 0 || rowIndex >= t.maxRowNumber {
+ return fmt.Errorf("illegal argument rowIndex %d", rowIndex)
+ }
+ if t.measurementSchemas[columnIndex].DataType != OBJECT {
+ return fmt.Errorf("column %d must be of type OBJECT",
columnIndex)
+ }
+ value := make([]byte, len(content)+9)
+ if isEOF {
+ value[0] = 1
+ }
+ binary.BigEndian.PutUint64(value[1:9], uint64(offset))
+ copy(value[9:], content)
+ return t.SetValueAt(value, columnIndex, rowIndex)
Review 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.
##########
client/rpcdataset.go:
##########
@@ -542,7 +542,7 @@ func (s *IoTDBRpcDataSet)
getObjectByTsBlockIndex(tsBlockColumnIndex int32) (int
} else {
return binary.GetStringValue(), nil
}
- case BLOB:
+ case BLOB, OBJECT:
Review 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`.
##########
common/common.go:
##########
@@ -475,6 +475,10 @@ const (
TAggregationType_SKEWNESS TAggregationType = 38
TAggregationType_KURTOSIS TAggregationType = 39
TAggregationType_PERCENTILE TAggregationType = 40
+ TAggregationType_RATE TAggregationType = 41
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
