Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New data type #12477

Closed
wants to merge 20 commits into from
Closed

New data type #12477

wants to merge 20 commits into from
+1,233 −220

Conversation

Cpaulyz
Copy link
Contributor

@Cpaulyz Cpaulyz commented May 7, 2024

Description

Content1 ...

Content2 ...

Content3 ...

This PR has:

  • been self-reviewed.
    • concurrent read
    • concurrent write
    • concurrent read and write
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods.
  • added or updated version, license, or notice information
  • added comments explaining the "why" and the intent of the code wherever would not be obvious
    for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold
    for code coverage.
  • added integration tests.
  • been tested in a test IoTDB cluster.
Key changed/added classes (or packages if there are too many classes) in this PR

@Cpaulyz Cpaulyz marked this pull request as ready for review May 17, 2024 07:17
JackieTien97
JackieTien97 requested changes May 20, 2024
View reviewed changes
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/IoTDBJDBCDataSet.java Outdated
RpcUtils.DEFAULT_TIME_FORMAT,
"ms",
BytesUtils.bytesToLong(values[index]),
ZoneId.systemDefault());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should add a zoneId field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/IoTDBDescriptor.java Outdated Show resolved Hide resolved
...-core/datanode/src/main/java/org/apache/iotdb/db/protocol/rest/utils/InsertRowDataUtils.java
@@ -95,6 +97,8 @@ public static List<Object> reGenValues(List<TSDataType> types, List<Object> valu
}
break;
case TEXT:
case BLOB:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for Blob, I think they pass String like "X'xaf'", so we need to parse that String.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not used RestAPI for writing. I need to go through some examples or unit tests to help me with this part later.

...src/main/java/org/apache/iotdb/db/protocol/rest/v1/handler/StatementConstructionHandler.java
@@ -140,6 +142,8 @@ public static InsertTabletStatement constructInsertTabletStatement(
columns[columnIndex] = doubleValues;
break;
case TEXT:
case BLOB:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should parse blob

...src/main/java/org/apache/iotdb/db/protocol/rest/v2/handler/StatementConstructionHandler.java
@@ -147,6 +149,8 @@ public static InsertTabletStatement constructInsertTabletStatement(
columns[columnIndex] = doubleValues;
break;
case TEXT:
case BLOB:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should parse blob

...e/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/statement/literal/Literal.java
@@ -36,7 +36,8 @@ public enum LiteralType {
DOUBLE,
LONG,
STRING,
NULL
NULL,
BINARY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't you add a switch-case in deserialize method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. It seems that Literal doesn't get serialised and deserialised when writing with sql, so I didn't notice.

...anode/src/main/java/org/apache/iotdb/db/queryengine/plan/statement/crud/InsertStatement.java Outdated
@@ -43,7 +43,7 @@ public class InsertStatement extends Statement {
private long[] times;
private String[] measurementList;

private List<String[]> valuesList;
private List<Object[]> valuesList;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why here need to be changed to Object[], it seems that all the caller is set String[]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forget why I modified this. But it looks like String[] is correct. I REVERT this modification.

HTHou
HTHou reviewed May 21, 2024
View reviewed changes
Copy link
Contributor

@HTHou HTHou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, there are some code in TsFileProcessor I think need to be considered.

iotdb/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/memtable/TsFileProcessor.java

Line 592 in b030642

if (dataTypes[i] == TSDataType.TEXT && values[i] != null) {

@Cpaulyz
Copy link
Contributor Author

Cpaulyz commented May 21, 2024

Hi, there are some code in TsFileProcessor I think need to be considered.

iotdb/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/memtable/TsFileProcessor.java

Line 592 in b030642

if (dataTypes[i] == TSDataType.TEXT && values[i] != null) {

@HTHou Thanks for the reminder! I missed some checks on TEXT. I made some changes in commit 228bba7. Please help review them for me~

I think adding an isBinary() method to TSDataType would have been a better choice, but that would have required to modify the TsFile repository.

@HTHou
Copy link
Contributor

HTHou commented May 22, 2024

Hi, there are some code in TsFileProcessor I think need to be considered.

iotdb/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/memtable/TsFileProcessor.java

Line 592 in b030642

if (dataTypes[i] == TSDataType.TEXT && values[i] != null) {

@HTHou Thanks for the reminder! I missed some checks on TEXT. I made some changes in commit 228bba7. Please help review them for me~

I think adding an isBinary() method to TSDataType would have been a better choice, but that would have required to modify the TsFile repository.

+1

@Cpaulyz
Copy link
Contributor Author

Cpaulyz commented May 23, 2024

Hi all, I will close this PR and open a new one for cooperation: #12576

@Cpaulyz Cpaulyz closed this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HTHou HTHou HTHou left review comments

@JackieTien97 JackieTien97 JackieTien97 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Cpaulyz @HTHou @JackieTien97