You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2022/03/01 11:09:13 UTC
[GitHub] [incubator-doris] yangzhg opened a new pull request #8289: [refactor] remove types_test
yangzhg opened a new pull request #8289:
URL: https://github.com/apache/incubator-doris/pull/8289
# Proposed changes
1. remove types_test, it will cause core dump in higher version gcc or
clang, becuse of memory align, some code will be vectorize in higher
gcc or clang
2. change string type length to 2GB instead of -1
3. modify unaccessable code
## Problem Summary:
Describe the overview of changes.
## Checklist(Required)
1. Does it affect the original behavior: (No)
2. Has unit tests been added: (No Need)
3. Has document been added or modified: (No Need)
4. Does it need to update dependencies: (No)
5. Are there any changes that cannot be rolled back: (No)
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] yangzhg merged pull request #8289: [refactor] remove types_test
Posted by GitBox <gi...@apache.org>.
yangzhg merged pull request #8289:
URL: https://github.com/apache/incubator-doris/pull/8289
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] HappenLee commented on a change in pull request #8289: [refactor] remove types_test
Posted by GitBox <gi...@apache.org>.
HappenLee commented on a change in pull request #8289:
URL: https://github.com/apache/incubator-doris/pull/8289#discussion_r817311314
##########
File path: fe/fe-core/src/main/java/org/apache/doris/catalog/ScalarType.java
##########
@@ -69,6 +66,12 @@
// Keep consistent with backend ColumnType::CHAR_INLINE_LENGTH
public static final int CHAR_INLINE_LENGTH = 128;
+ // Max length of String types, in be storage layer store string length
+ // using int32, the max length is 2GB, the first 4 bytes store the length
+ // so the max available length is 2GB - 4
+
Review comment:
no need the space？
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] levy5307 commented on a change in pull request #8289: [refactor] remove types_test
Posted by GitBox <gi...@apache.org>.
levy5307 commented on a change in pull request #8289:
URL: https://github.com/apache/incubator-doris/pull/8289#discussion_r817329249
##########
File path: fe/fe-core/src/main/java/org/apache/doris/catalog/ScalarType.java
##########
@@ -69,6 +66,12 @@
// Keep consistent with backend ColumnType::CHAR_INLINE_LENGTH
public static final int CHAR_INLINE_LENGTH = 128;
+ // Max length of String types, in be storage layer store string length
+ // using int32, the max length is 2GB, the first 4 bytes store the length
+ // so the max available length is 2GB - 4
+
+ public static final int MAX_STRING_LENGTH = 0x7fffffff - 4;
Review comment:
I think `public static final int MAX_STRING_LENGTH = 2 << 30 - 1 - 4;` is more clear.
And you should explain why did you minus 1 here.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] yangzhg commented on a change in pull request #8289: [refactor] remove types_test
Posted by GitBox <gi...@apache.org>.
yangzhg commented on a change in pull request #8289:
URL: https://github.com/apache/incubator-doris/pull/8289#discussion_r817610939
##########
File path: fe/fe-core/src/main/java/org/apache/doris/catalog/ScalarType.java
##########
@@ -69,6 +66,12 @@
// Keep consistent with backend ColumnType::CHAR_INLINE_LENGTH
public static final int CHAR_INLINE_LENGTH = 128;
+ // Max length of String types, in be storage layer store string length
+ // using int32, the max length is 2GB, the first 4 bytes store the length
+ // so the max available length is 2GB - 4
+
+ public static final int MAX_STRING_LENGTH = 0x7fffffff - 4;
Review comment:
It is explained in comments that the value is 2GB - 4, why use `public static final int MAX_STRING_LENGTH = 2 << 30 - 1 - 4;` ? How to compute 2GB - 4 is not important and 2 << 30 does not have performance advantages
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] levy5307 commented on a change in pull request #8289: [refactor] remove types_test
Posted by GitBox <gi...@apache.org>.
levy5307 commented on a change in pull request #8289:
URL: https://github.com/apache/incubator-doris/pull/8289#discussion_r817329249
##########
File path: fe/fe-core/src/main/java/org/apache/doris/catalog/ScalarType.java
##########
@@ -69,6 +66,12 @@
// Keep consistent with backend ColumnType::CHAR_INLINE_LENGTH
public static final int CHAR_INLINE_LENGTH = 128;
+ // Max length of String types, in be storage layer store string length
+ // using int32, the max length is 2GB, the first 4 bytes store the length
+ // so the max available length is 2GB - 4
+
+ public static final int MAX_STRING_LENGTH = 0x7fffffff - 4;
Review comment:
I think `public static final int MAX_STRING_LENGTH = 2 << 30 - 1 - 4;` is more clear here.
And you should explain why did you minus 1 here.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] github-actions[bot] commented on pull request #8289: [refactor] remove types_test
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8289:
URL: https://github.com/apache/incubator-doris/pull/8289#issuecomment-1056898923
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] yangzhg commented on a change in pull request #8289: [refactor] remove types_test
Posted by GitBox <gi...@apache.org>.
yangzhg commented on a change in pull request #8289:
URL: https://github.com/apache/incubator-doris/pull/8289#discussion_r817610939
##########
File path: fe/fe-core/src/main/java/org/apache/doris/catalog/ScalarType.java
##########
@@ -69,6 +66,12 @@
// Keep consistent with backend ColumnType::CHAR_INLINE_LENGTH
public static final int CHAR_INLINE_LENGTH = 128;
+ // Max length of String types, in be storage layer store string length
+ // using int32, the max length is 2GB, the first 4 bytes store the length
+ // so the max available length is 2GB - 4
+
+ public static final int MAX_STRING_LENGTH = 0x7fffffff - 4;
Review comment:
It is explained in comments that the value is 2GB - 4, why use `public static final int MAX_STRING_LENGTH = 2 << 30 - 1 - 4;` ? How to compute 2GB - 4 is not important and 2 << 30 does not have any performance or readability advantages
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request #8289: [refactor] remove types_test
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #8289:
URL: https://github.com/apache/incubator-doris/pull/8289#discussion_r816785983
##########
File path: fe/fe-core/src/main/java/org/apache/doris/catalog/ScalarType.java
##########
@@ -291,7 +291,8 @@ public static ScalarType createVarcharType(String lenStr) {
public static ScalarType createStringType() {
// length checked in analysis
ScalarType type = new ScalarType(PrimitiveType.STRING);
- type.len = -1;
+ // 2GB - 4
+ type.len = 2147483643;
Review comment:
Define this magic number some where. Maybe in Type.java?
And explain why minus 4 in comment.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org