-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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.
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
[Feature] Z-Order Implement #7149
Conversation
Nice work! |
apache carbondata also has this min/max index,nice |
b588425
to
fc81ccd
Compare
be/test/olap/skiplist_test.cpp
Outdated
@@ -268,7 +271,7 @@ class ConcurrentTest { | |||
ConcurrentTest() | |||
: _mem_tracker(new MemTracker(-1)), | |||
_mem_pool(new MemPool(_mem_tracker.get())), | |||
_list(TestComparator(), _mem_pool.get(), false) {} | |||
_list(new TestComparator(), _mem_pool.get(), false) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add ~ConcurrentTest()
to delete _list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_list is an object, is it needs to delete it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you pass a raw pointer to the skiplist, and nowhere to delete this TestComparator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
fe/fe-core/src/main/java/org/apache/doris/analysis/DataSortInfo.java
Outdated
Show resolved
Hide resolved
return true; | ||
} | ||
|
||
public String getProperties() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toString() maybe better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getProperties keeps the same style with before, like dynamic partition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, So I think we should change the dynamic partition too...
It is more like toString() or toSql()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modify it to toSql()
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java
Outdated
Show resolved
Hide resolved
@@ -137,6 +151,11 @@ public void modifyTableProperties(Map<String, String> modifyProperties) { | |||
properties.putAll(modifyProperties); | |||
} | |||
|
|||
public void modifyTableProperties(DataSortInfo dataSortInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the method name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modifyTableProperties keeps the same style with before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not same, the origin 2 modifyTableProperties()
method pass the key-value
, which means it can modify any of table properties.
But here you just want to modify the data sort info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modify it to modifyDataSortInfoProperties()
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java
Outdated
Show resolved
Hide resolved
@@ -165,6 +202,10 @@ public TCreateTabletReq toThrift() { | |||
tSchema.setSchemaHash(schemaHash); | |||
tSchema.setKeysType(keysType.toThrift()); | |||
tSchema.setStorageType(storageType); | |||
if (dataSortInfo != null) { | |||
tSchema.setSortType(dataSortInfo.getSortType()); | |||
tSchema.setSortColNum(dataSortInfo.getColNum()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use default value instead of optional setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dataSortInfo maybe null, will cause toThrift() failed
I tested my own and it works well.
|
282981f
to
d677397
Compare
return true; | ||
} | ||
|
||
public String getProperties() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, So I think we should change the dynamic partition too...
It is more like toString() or toSql()
public boolean isZOrderSort() { | ||
return tableProperty != null | ||
&& tableProperty.getDataSortInfo() != null | ||
&& tableProperty.getDataSortInfo().getSortType() != TSortType.LEXICAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&& tableProperty.getDataSortInfo().getSortType() != TSortType.LEXICAL; | |
&& tableProperty.getDataSortInfo().getSortType() == TSortType.ZORDER; |
So that we don't need to modify it if we add some other sort type later.
@@ -1714,6 +1729,13 @@ public TStorageFormat getStorageFormat() { | |||
return tableProperty.getStorageFormat(); | |||
} | |||
|
|||
public DataSortInfo getDataSortInfo() { | |||
if (tableProperty == null) { | |||
return new DataSortInfo(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better assign the default value to the fields of DataSortInfo
, so that we can make this return more semantically accurate
@@ -137,6 +151,11 @@ public void modifyTableProperties(Map<String, String> modifyProperties) { | |||
properties.putAll(modifyProperties); | |||
} | |||
|
|||
public void modifyTableProperties(DataSortInfo dataSortInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not same, the origin 2 modifyTableProperties()
method pass the key-value
, which means it can modify any of table properties.
But here you just want to modify the data sort info.
fe/fe-core/src/main/java/org/apache/doris/analysis/DataSortInfo.java
Outdated
Show resolved
Hide resolved
documents are added in 'create table' doc; Alter table is prohibited for z-order table |
bcb86e6
to
a7cecc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
4e70bb2
to
3bc0a9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR approved by at least one committer and no changes requested. |
Proposed changes
For issue: #6359
Background
Z-Order
Application Situation
Grammar
data_sort.sort_type: support lexical/z-order sort type, default is lexical sort type
data_sort.col_num: take the pre-columns as sort key
Performance Test
Load Performance
Env: ssb scale 100, stream load
Query Performance
Env: TPCH scale 25
Table:
Q1: select count(1) from LINEITEM where L_PARTKEY=999852;
Q2: select count(1) from LINEITEM where L_SUPPKEY=125019;
Limitation
Future work
Types of changes
What types of changes does your code introduce to Doris?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...