fix: fix EOF error when decoding columns with empty string#154
fix: fix EOF error when decoding columns with empty string#154betterlmy wants to merge 1 commit intoapache:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an EOF error encountered when decoding TEXT columns containing empty strings in TsBlock results, and adds a dataset helper for retrieving the current row’s raw timestamp value.
Changes:
- Fix TEXT decoding to avoid triggering
EOFwhen a value haslength == 0(empty string). - Handle
positionCount == 0in multiple column decoders by returning empty columns without reading from the buffer. - Add
SessionDataSet.GetCurrentRowTime()passthrough to expose the current row’s raw timestamp value, and add decoder-focused tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| client/column_decoder.go | Adjusts decoding logic for empty TEXT values and zero-position columns. |
| client/column_decoder_test.go | Adds unit tests covering empty-string TEXT decoding and zero-position decoding. |
| client/session.go | Adds nil-response guarding in several reconnect retry paths for query-related RPCs. |
| client/sessiondataset.go | Adds GetCurrentRowTime() API to expose the current row time from the underlying RPC dataset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var length int32 | ||
| err := binary.Read(reader, binary.BigEndian, &length) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| value := make([]byte, length) | ||
| _, err = reader.Read(value) | ||
| if err != nil { | ||
| return nil, err | ||
|
|
||
| if length == 0 { | ||
| values[i] = NewBinary([]byte{}) | ||
| } else { | ||
| value := make([]byte, length) | ||
| _, err = reader.Read(value) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
length is read from the server and can be negative on malformed/corrupted input; make([]byte, length) will panic in that case. Also, reader.Read(value) doesn’t guarantee filling the whole buffer (it can return a short read with nil error), which could silently truncate TEXT values. Consider validating length >= 0 and using a full-read (e.g., io.ReadFull) for length > 0 to either read exactly length bytes or return an error.
| if err == nil && resp != nil { | ||
| if statusErr := VerifySuccess(resp.Status); statusErr == nil { | ||
| return NewSessionDataSet(sql, resp.Columns, resp.DataTypeList, resp.ColumnNameIndexMap, *resp.QueryId, s.requestStatementId, s.client, s.sessionId, resp.QueryResult_, resp.IgnoreTimeStamp != nil && *resp.IgnoreTimeStamp, timeoutMs, *resp.MoreData, s.config.FetchSize, s.config.TimeZone, s.timeFactor, resp.GetColumnIndex2TsBlockColumnIndexList()) | ||
| } else { | ||
| return nil, statusErr | ||
| } | ||
| } |
There was a problem hiding this comment.
The new if err == nil && resp != nil { ... } guard avoids a panic, but if err == nil and resp == nil this function will fall through and return (nil, nil) (since the final return nil, err returns a nil error). Please handle the resp == nil case explicitly (both for the initial RPC call and the reconnect retry) and return a non-nil error when the RPC returns a nil response.
| if err == nil && resp != nil { | ||
| if statusErr := VerifySuccess(resp.Status); statusErr == nil { | ||
| return NewSessionDataSet("", resp.Columns, resp.DataTypeList, resp.ColumnNameIndexMap, *resp.QueryId, s.requestStatementId, s.client, s.sessionId, resp.QueryResult_, resp.IgnoreTimeStamp != nil && *resp.IgnoreTimeStamp, timeoutMs, *resp.MoreData, s.config.FetchSize, s.config.TimeZone, s.timeFactor, resp.GetColumnIndex2TsBlockColumnIndexList()) | ||
| } else { | ||
| return nil, statusErr | ||
| } | ||
| } |
There was a problem hiding this comment.
Same issue here: if the retry call returns err == nil but resp == nil, the function will fall through and return (nil, nil). Please add explicit handling for nil responses (and ideally apply the same nil-response check on the initial RPC call too) so callers never get a nil dataset with a nil error.
| if err == nil && resp != nil { | ||
| if statusErr := VerifySuccess(resp.Status); statusErr == nil { | ||
| return NewSessionDataSet("", resp.Columns, resp.DataTypeList, resp.ColumnNameIndexMap, *resp.QueryId, s.requestStatementId, s.client, s.sessionId, resp.QueryResult_, resp.IgnoreTimeStamp != nil && *resp.IgnoreTimeStamp, timeoutMs, *resp.MoreData, s.config.FetchSize, s.config.TimeZone, s.timeFactor, resp.GetColumnIndex2TsBlockColumnIndexList()) | ||
| } else { | ||
| return nil, statusErr | ||
| } | ||
| } |
There was a problem hiding this comment.
Same nil-response handling problem as above: the retry path now checks resp != nil, but if err == nil and resp == nil it will return (nil, nil). Please return an explicit error when the RPC returns a nil response (and consider guarding the initial non-retry call similarly).
| if err == nil && resp != nil { | ||
| if statusErr := VerifySuccess(resp.Status); statusErr == nil { | ||
| return NewSessionDataSet("", resp.Columns, resp.DataTypeList, resp.ColumnNameIndexMap, *resp.QueryId, s.requestStatementId, s.client, s.sessionId, resp.QueryResult_, resp.IgnoreTimeStamp != nil && *resp.IgnoreTimeStamp, timeoutMs, *resp.MoreData, s.config.FetchSize, s.config.TimeZone, s.timeFactor, resp.GetColumnIndex2TsBlockColumnIndexList()) | ||
| } else { | ||
| return nil, statusErr | ||
| } | ||
| } |
There was a problem hiding this comment.
Same issue in this retry path: err == nil && resp == nil will cause the method to return (nil, nil) due to the final return nil, err. Please explicitly treat a nil response as an error (and consider applying the same check on the initial RPC call as well).
| func (s *SessionDataSet) GetCurrentRowTime() int64 { | ||
| return s.ioTDBRpcDataSet.GetCurrentRowTime() |
There was a problem hiding this comment.
IoTDBRpcDataSet.GetCurrentRowTime() returns the raw s.time value, whose unit depends on the server time precision (ms/us/ns) negotiated via timeFactor. If this API is intended to return milliseconds as stated in the PR description, it should convert using the configured time precision (or otherwise document clearly that it returns the raw precision-dependent value).
e92e404 to
0da470a
Compare
Bug 复现与修复报告
一、问题描述
在使用 IoTDB Go Client 查询包含空字符串(
"")的 TEXT 列时,客户端报EOF错误,导致查询失败。同一条 SQL 在 IoTDB 命令行中执行正常。二、复现步骤
环境: IoTDB 2.0.5
第一步:写入一条 TEXT 列值为空字符串的数据
第二步:用 Go Client 查询该列
复现条件:
""(非 null)报错信息:
三、根本原因分析
问题出在
client/column_decoder.go的BinaryArrayColumnDecoder.ReadColumn。IoTDB 服务端对 TEXT 类型的每个值序列化格式为:
当值为空字符串时,服务端发送
length = 0,后跟 0 字节内容。旧代码:
Go 标准库
bytes.Reader.Read在 reader 已无剩余字节时,即使传入空 slice(读取 0 字节),也会返回io.EOF。当空字符串恰好是 TsBlock 字节流中最后一个值时,reader.Read([]byte{})触发 EOF,导致解码失败。同时发现的第二个 bug: 当服务端返回
positionCount = 0的列时,所有类型的 ColumnDecoder 都会调用deserializeNullIndicators,后者对空 reader 执行ReadByte(),同样返回io.EOF。四、修复方案
Bug 1(空字符串 EOF): 在
BinaryArrayColumnDecoder中,当length == 0时直接创建空 Binary,跳过reader.Read:Bug 2(positionCount=0 EOF): 在四种 ColumnDecoder 的
ReadColumn入口处,对positionCount == 0提前返回空 Column:五、验证结果
修复前:
修复后:
六、新增 Feature:SessionDataSet.GetCurrentRowTime()
SessionDataSet新增GetCurrentRowTime()方法,返回当前行的原始时间戳(int64,毫秒级 Unix 时间戳)。背景: 原有的
GetTimestamp(columnName)方法返回time.Time,需要指定列名,且经过时区转换。对于需要直接操作原始时间戳数值的场景(如比较、存储、序列化),需要再做一次转换,不够便捷。用法: