Copilot commented on code in PR #154:
URL: https://github.com/apache/iotdb-client-go/pull/154#discussion_r2905541002
##########
client/column_decoder.go:
##########
@@ -232,12 +266,17 @@ func (decoder *BinaryArrayColumnDecoder)
ReadColumn(reader *bytes.Reader, dataTy
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
Review 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.
##########
client/session.go:
##########
@@ -569,10 +569,12 @@ func (s *Session) ExecuteQueryStatement(sql string,
timeoutMs *int64) (*SessionD
request.SessionId = s.sessionId
request.StatementId = s.requestStatementId
resp, err =
s.client.ExecuteQueryStatementV2(context.Background(), &request)
- 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
+ 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
+ }
}
Review 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.
##########
client/session.go:
##########
@@ -626,10 +630,12 @@ func (s *Session)
ExecuteAggregationQueryWithLegalNodes(paths []string, aggregat
if s.reconnect() {
request.SessionId = s.sessionId
resp, err =
s.client.ExecuteAggregationQueryV2(context.Background(), &request)
- 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
+ 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
+ }
}
Review 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).
##########
client/sessiondataset.go:
##########
@@ -125,3 +125,7 @@ func (s *SessionDataSet) GetColumnNames() []string {
func (s *SessionDataSet) GetColumnTypes() []string {
return s.ioTDBRpcDataSet.columnTypeList
}
+
+func (s *SessionDataSet) GetCurrentRowTime() int64 {
+ return s.ioTDBRpcDataSet.GetCurrentRowTime()
Review 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).
##########
client/session.go:
##########
@@ -597,10 +599,12 @@ func (s *Session) ExecuteAggregationQuery(paths []string,
aggregations []common.
if s.reconnect() {
request.SessionId = s.sessionId
resp, err =
s.client.ExecuteAggregationQueryV2(context.Background(), &request)
- 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
+ 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
+ }
}
Review 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.
##########
client/session.go:
##########
@@ -653,10 +659,12 @@ func (s *Session)
ExecuteFastLastDataQueryForOnePrefixPath(prefixes []string, ti
if s.reconnect() {
request.SessionId = s.sessionId
resp, err =
s.client.ExecuteFastLastDataQueryForOnePrefixPath(context.Background(),
&request)
- 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
+ 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
+ }
}
Review 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).
--
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]
