You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2021/07/20 15:14:39 UTC
[GitHub] [iotdb] ericpai opened a new pull request #3605: [IOTDB-1515]Fix binary convertion in LastQueryExecutor
ericpai opened a new pull request #3605:
URL: https://github.com/apache/iotdb/pull/3605
Please see JIRA https://issues.apache.org/jira/browse/IOTDB-1515 .
Here are another two questions. Any guidance or discussion is welcomed :).
1. What is the `Binary` type designed for? Any bytes or only valid UTF-8 strings?
2. If any bytes is allowed, the implementation of `AbstractIoTDBJDBCResultSet.getBytes` is needed. As we cannot restore orignal binary data from a malformed UTF-8 string.
If this PR is accepted. I think it should be cherry-picked to release branches.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] ericpai edited a comment on pull request #3605: [IOTDB-1515]Fix binary convertion in LastQueryExecutor
Posted by GitBox <gi...@apache.org>.
ericpai edited a comment on pull request #3605:
URL: https://github.com/apache/iotdb/pull/3605#issuecomment-883800383
> some modification of insertRecord is needed I think…
@HTHou Yes, InsertRecord treats the input `TEXT` type data as a `String`, not `byte[]`. But user will be confused if he uses `InsertTablet` API to insert Binary objects and gets unexcepted ones.
Maybe we can indicate that only valid UTF-8 String supported in TEXT type in `InsertRecord()`, and it will support any bytes in the future. It's backward compatible I think. But if we want to support any bytes, the JDBC interface should implement `getBytes()`. I could help to do this.
However this PR is the superset of UTF-8 string support(I can add another test case for valid utf8 bytes). Do you think it should be merged?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] ericpai commented on pull request #3605: [IOTDB-1515]Fix binary convertion in LastQueryExecutor
Posted by GitBox <gi...@apache.org>.
ericpai commented on pull request #3605:
URL: https://github.com/apache/iotdb/pull/3605#issuecomment-883800383
> some modification of insertRecord is needed I think…
@HTHou Yes, InsertRecord treats the input `TEXT` type data as a `String`, not `byte[]`. But user will be confused if it uses InsertTablet API to insert Binary objects and gets unexcepted ones.
Maybe we can indicate that only valid UTF-8 String supported in TEXT type in `InsertRecord()`, and support any bytes in the future. It's backward compatiable I think. But if we want to support any bytes. The JDBC interface should support `getBytes()`. I could help to do this.
However this PR is the superset of UTF-8 string support(I can add another test case for valid utf8 bytes). Do you think it should be merged?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] coveralls commented on pull request #3605: [IOTDB-1515]Fix binary convertion in LastQueryExecutor
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #3605:
URL: https://github.com/apache/iotdb/pull/3605#issuecomment-883542032
[](https://coveralls.io/builds/41512867)
Coverage decreased (-0.01%) to 68.152% when pulling **575c95bed78da92def69bbe26f4da92429a1c99a on ericpai:bugfix/last-query-binary** into **79e50d651a2b488ac211009947b2a01c5244c37b on apache:master**.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] jixuan1989 commented on pull request #3605: [IOTDB-1515]Fix binary convertion in LastQueryExecutor
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on pull request #3605:
URL: https://github.com/apache/iotdb/pull/3605#issuecomment-887136316
@ericpai , we indeed need a new binary (better to be called as Blob) data type, which stores bytes and has no statistics like sum etc (first value and last value are also not needed by default)..
Then, current binary can be renamed as Clob or Text directly (but we need to consider compatibility)
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] qiaojialin commented on pull request #3605: [IOTDB-1515]Fix binary convertion in LastQueryExecutor
Posted by GitBox <gi...@apache.org>.
qiaojialin commented on pull request #3605:
URL: https://github.com/apache/iotdb/pull/3605#issuecomment-883860903
Hi, the data type of Binary is TEXT, so it's before only store UTF-8 String.
If we want to store bytes, we could import another data type called BINARY.
This could be discussed in the mail list :)
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] coveralls edited a comment on pull request #3605: [IOTDB-1515]Fix binary convertion in LastQueryExecutor
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #3605:
URL: https://github.com/apache/iotdb/pull/3605#issuecomment-883542032
[](https://coveralls.io/builds/41525687)
Coverage decreased (-0.01%) to 68.151% when pulling **01cc43d6314ca4a1bcbc07ef81095f573bfaedb0 on ericpai:bugfix/last-query-binary** into **79e50d651a2b488ac211009947b2a01c5244c37b on apache:master**.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] HTHou edited a comment on pull request #3605: [IOTDB-1515]Fix binary convertion in LastQueryExecutor
Posted by GitBox <gi...@apache.org>.
HTHou edited a comment on pull request #3605:
URL: https://github.com/apache/iotdb/pull/3605#issuecomment-883501559
Hi, currently we can only insert any bytes with Binary by insertTablet interface, right? InsertRecord interface only support insert String record. If we consider to support any bytes, some modification of insertRecord is needed I think…
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] ericpai closed pull request #3605: [IOTDB-1515]Fix binary convertion in LastQueryExecutor
Posted by GitBox <gi...@apache.org>.
ericpai closed pull request #3605:
URL: https://github.com/apache/iotdb/pull/3605
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] ericpai commented on pull request #3605: [IOTDB-1515]Fix binary convertion in LastQueryExecutor
Posted by GitBox <gi...@apache.org>.
ericpai commented on pull request #3605:
URL: https://github.com/apache/iotdb/pull/3605#issuecomment-883864507
> Hi, the data type of Binary is TEXT, so it's before only store UTF-8 String.
>
> If we want to store bytes, we could import another data type called BINARY.
>
> This could be discussed in the mail list :)
It makes sense! Currently I will use base64 encoding as a workaround solution to store arbitrary bytes. This PR and JIRA will be closed.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] HTHou commented on pull request #3605: [IOTDB-1515]Fix binary convertion in LastQueryExecutor
Posted by GitBox <gi...@apache.org>.
HTHou commented on pull request #3605:
URL: https://github.com/apache/iotdb/pull/3605#issuecomment-883501559
Hi, currently we can only insert any bytes with Binary by insertTablet interface, right? InsertRecord interface only support insert String record. If we consider to support any bytes, a new interface of insertRecord is needed I think…
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org