Fix C++ client time column access returning NULL for non-long types (#17397)#17400
Open
PDGGK wants to merge 1 commit intoapache:masterfrom
Open
Fix C++ client time column access returning NULL for non-long types (#17397)#17400PDGGK wants to merge 1 commit intoapache:masterfrom
PDGGK wants to merge 1 commit intoapache:masterfrom
Conversation
…pache#17397) Several ByTsBlockColumnIndex methods in IoTDBRpcDataSet did not handle the tsBlockColumnIndex < 0 case (time column), causing undefined behavior when accessing the time column as boolean, double, float, int, or binary. Only getLong and getString handled this correctly. Add tsBlockColumnIndex < 0 checks to: - getBooleanByTsBlockColumnIndex: throw IoTDBException (boolean from timestamp is nonsensical) - getDoubleByTsBlockColumnIndex: return timestamp cast to double - getFloatByTsBlockColumnIndex: return timestamp cast to float - getIntByTsBlockColumnIndex: return timestamp cast to int32_t - getBinaryByTsBlockColumnIndex: return timestamp as Binary string Signed-off-by: Zihan Dai <1436286758@qq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #17397
Several
ByTsBlockColumnIndexgetter methods inIoTDBRpcDataSet.cppdid not handle thetsBlockColumnIndex < 0case (time pseudo-column in tree model), causing undefined behavior when the time column was accessed through typed getters other thangetLongorgetString.In tree model, column index 1 maps to
tsBlockColumnIndex = -1(the time column). When a user callsgetBooleanByIndex(1), this flows togetBooleanByTsBlockColumnIndex(-1), which callscurTsBlock_->getColumn(-1)— an out-of-boundsstd::vectoraccess (UB in C++).Only
getLongByTsBlockColumnIndexandgetStringByTsBlockColumnIndexcorrectly handled the< 0case. The following five methods were missing the guard:getBooleanByTsBlockColumnIndexIoTDBException(boolean from timestamp is nonsensical)getDoubleByTsBlockColumnIndexdoublegetFloatByTsBlockColumnIndexfloatgetIntByTsBlockColumnIndexint32_tgetBinaryByTsBlockColumnIndexBinarystringDesign Discussion
For the numeric types (
int,float,double), I chose to return the timestamp value (cast to the target type) to stay consistent withgetLongByTsBlockColumnIndexwhich already returns the timestamp. However, an alternative approach would be to throwIoTDBExceptionfor all non-long/non-string types, since:static_cast<int32_t>(timestamp)silently truncates 64-bit timestampsfloatorbooleanis likely a user errorI'd appreciate the maintainers' preference on which approach is better. Happy to change if throwing is preferred.
Note
The Java client (
IoTDBRpcDataSet.java) has the same asymmetry — onlygetLongandgetStringhandletsBlockColumnIndex < 0. The other typed getters (getBoolean,getInt,getFloat,getDouble,getBinary) will throwIndexOutOfBoundsExceptionin Java. This could be addressed as a follow-up.