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/08/26 13:50:13 UTC
[GitHub] [doris] dataroaring opened a new pull request, #12117: [fix](storage) storage layer should aggregate on replace column
dataroaring opened a new pull request, #12117:
URL: https://github.com/apache/doris/pull/12117
# Proposed changes
Issue Number: close #xxx
## Problem summary
Describe your changes.
## Checklist(Required)
1. Does it affect the original behavior:
- [ ] Yes
- [ ] No
- [ ] I don't know
2. Has unit tests been added:
- [ ] Yes
- [ ] No
- [ ] No Need
3. Has document been added or modified:
- [ ] Yes
- [ ] No
- [ ] No Need
4. Does it need to update dependencies:
- [ ] Yes
- [ ] No
5. Are there any changes that cannot be rolled back:
- [ ] Yes (If Yes, please explain WHY)
- [ ] 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] [doris] zhannngchen commented on a diff in pull request #12117: [fix](storage) storage layer should aggregate on replace column
Posted by GitBox <gi...@apache.org>.
zhannngchen commented on code in PR #12117:
URL: https://github.com/apache/doris/pull/12117#discussion_r956531633
##########
be/src/exec/olap_scanner.cpp:
##########
@@ -152,14 +152,15 @@ Status OlapScanner::_init_tablet_reader_params(
->rowset()
->rowset_meta()
->is_segments_overlapping());
+
+ bool has_replace_col = false;
+ RETURN_IF_ERROR(_init_return_columns(!single_non_overlapping_version, has_replace_col));
- _tablet_reader_params.direct_mode = single_version || _aggregation;
Review Comment:
Is the following simple change also can fix the issue?
```
bool unique_key_with_seq = _tablet->keys_type() == UNIQUE_KEYS && _tablet->tablet_schema().has_sequence_col();
_tablet_reader_params.direct_mode = single_version || (_aggregation && ! unique_key_with_seq) ;
```
--
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] [doris] github-actions[bot] commented on pull request #12117: [fix](storage) storage layer should aggregate on replace column
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12117:
URL: https://github.com/apache/doris/pull/12117#issuecomment-1229099962
PR approved by anyone and no changes requested.
--
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] [doris] zhannngchen commented on a diff in pull request #12117: [fix](storage) storage layer should aggregate on replace column
Posted by GitBox <gi...@apache.org>.
zhannngchen commented on code in PR #12117:
URL: https://github.com/apache/doris/pull/12117#discussion_r956531633
##########
be/src/exec/olap_scanner.cpp:
##########
@@ -152,14 +152,15 @@ Status OlapScanner::_init_tablet_reader_params(
->rowset()
->rowset_meta()
->is_segments_overlapping());
+
+ bool has_replace_col = false;
+ RETURN_IF_ERROR(_init_return_columns(!single_non_overlapping_version, has_replace_col));
- _tablet_reader_params.direct_mode = single_version || _aggregation;
Review Comment:
Is the following change also can fix the issue?
```
bool unique_key_with_seq = _tablet->keys_type() == UNIQUE_KEYS && _tablet->tablet_schema().has_sequence_col();
_tablet_reader_params.direct_mode = single_version || (_aggregation && ! unique_key_with_seq) ;
```
--
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] [doris] dataroaring commented on a diff in pull request #12117: [fix](storage) storage layer should aggregate on replace column
Posted by GitBox <gi...@apache.org>.
dataroaring commented on code in PR #12117:
URL: https://github.com/apache/doris/pull/12117#discussion_r956558085
##########
be/src/exec/olap_scanner.cpp:
##########
@@ -152,14 +152,15 @@ Status OlapScanner::_init_tablet_reader_params(
->rowset()
->rowset_meta()
->is_segments_overlapping());
+
+ bool has_replace_col = false;
+ RETURN_IF_ERROR(_init_return_columns(!single_non_overlapping_version, has_replace_col));
- _tablet_reader_params.direct_mode = single_version || _aggregation;
Review Comment:
I am not sure what we need to in planner. If fe sets aggregation, we ignore it here, then we can ignore aggreagate in planner somtimes? Could you throw an example sql?
--
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] [doris] dataroaring commented on a diff in pull request #12117: [fix](storage) storage layer should aggregate on replace column
Posted by GitBox <gi...@apache.org>.
dataroaring commented on code in PR #12117:
URL: https://github.com/apache/doris/pull/12117#discussion_r956583630
##########
be/src/exec/olap_scanner.cpp:
##########
@@ -152,14 +152,15 @@ Status OlapScanner::_init_tablet_reader_params(
->rowset()
->rowset_meta()
->is_segments_overlapping());
+
+ bool has_replace_col = false;
+ RETURN_IF_ERROR(_init_return_columns(!single_non_overlapping_version, has_replace_col));
- _tablet_reader_params.direct_mode = single_version || _aggregation;
Review Comment:
I suspect that columns aggregated by replace without SEQ does not work either.
--
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] [doris] github-actions[bot] commented on pull request #12117: [fix](storage) storage layer should aggregate on replace column
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12117:
URL: https://github.com/apache/doris/pull/12117#issuecomment-1229112315
PR approved by anyone and no changes requested.
--
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] [doris] dataroaring closed pull request #12117: [fix](storage) storage layer should aggregate on replace column
Posted by GitBox <gi...@apache.org>.
dataroaring closed pull request #12117: [fix](storage) storage layer should aggregate on replace column
URL: https://github.com/apache/doris/pull/12117
--
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] [doris] zhannngchen commented on a diff in pull request #12117: [fix](storage) storage layer should aggregate on replace column
Posted by GitBox <gi...@apache.org>.
zhannngchen commented on code in PR #12117:
URL: https://github.com/apache/doris/pull/12117#discussion_r956530960
##########
be/src/exec/olap_scanner.cpp:
##########
@@ -152,14 +152,15 @@ Status OlapScanner::_init_tablet_reader_params(
->rowset()
->rowset_meta()
->is_segments_overlapping());
+
+ bool has_replace_col = false;
+ RETURN_IF_ERROR(_init_return_columns(!single_non_overlapping_version, has_replace_col));
- _tablet_reader_params.direct_mode = single_version || _aggregation;
Review Comment:
The issue here is:
1. Planner thinks scanner can scan directly, don't need merge and can do some pre-aggregation works.
2. But scanner found it's a unqiue key table with seq column which need to merge first.
I think we'd better fix it in planner? Planner should not set preAggregation as true on a unique table with seq column?
--
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] [doris] zhannngchen commented on a diff in pull request #12117: [fix](storage) storage layer should aggregate on replace column
Posted by GitBox <gi...@apache.org>.
zhannngchen commented on code in PR #12117:
URL: https://github.com/apache/doris/pull/12117#discussion_r956848748
##########
be/src/exec/olap_scanner.cpp:
##########
@@ -152,14 +152,15 @@ Status OlapScanner::_init_tablet_reader_params(
->rowset()
->rowset_meta()
->is_segments_overlapping());
+
+ bool has_replace_col = false;
+ RETURN_IF_ERROR(_init_return_columns(!single_non_overlapping_version, has_replace_col));
- _tablet_reader_params.direct_mode = single_version || _aggregation;
Review Comment:
> I suspect that columns aggregated by replace without SEQ does not work either.
root cause is _aggregation should not be true in such situation?
--
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] [doris] zhannngchen commented on a diff in pull request #12117: [fix](storage) storage layer should aggregate on replace column
Posted by GitBox <gi...@apache.org>.
zhannngchen commented on code in PR #12117:
URL: https://github.com/apache/doris/pull/12117#discussion_r956530960
##########
be/src/exec/olap_scanner.cpp:
##########
@@ -152,14 +152,15 @@ Status OlapScanner::_init_tablet_reader_params(
->rowset()
->rowset_meta()
->is_segments_overlapping());
+
+ bool has_replace_col = false;
+ RETURN_IF_ERROR(_init_return_columns(!single_non_overlapping_version, has_replace_col));
- _tablet_reader_params.direct_mode = single_version || _aggregation;
Review Comment:
The issue here is:
1. Planner thinks scanner can scan directly, don't need merge and can do some pre-aggregation works.
2. But scanner found it's a unqiue key table with seq column which need to merge first.
I think we'd fix it in planner? Planner should not set preAggregation as true on a unique table with seq column?
--
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