Skip to content

[fix](planner) Fix wrong plan of SQL with cross join and aggreagate#12226

Closed
Kikyou1997 wants to merge 1 commit intoapache:masterfrom
Kikyou1997:fix_491
Closed

[fix](planner) Fix wrong plan of SQL with cross join and aggreagate#12226
Kikyou1997 wants to merge 1 commit intoapache:masterfrom
Kikyou1997:fix_491

Conversation

@Kikyou1997
Copy link
Contributor

@Kikyou1997 Kikyou1997 commented Aug 31, 2022

Proposed changes

Issue Number: noissue

Problem summary

SELECT col1
FROM 
    (SELECT col1 FROM t1) a
INNER JOIN 
    (SELECT sum(b3) AS 'b3u'
    FROM 
        (SELECT `c`.`col2` AS `c2`,
         sum(b.col3) AS `b3`
        FROM `t2` b
        INNER JOIN `t3` c
            ON ((`b`.`col1` = `c`.`col1`))
        GROUP BY  `c`.`col2`) dd ) b;

For such SQL type, the materialized attribute of SlotDescriptor will be updated during the optmization for countStar and cross join(see this method materializeInlineViewResultExprForCrossJoinOrCountStar), though the single node plan has been generated, and the generation of plan is dependent on the materialized attribute, so apparently there is a parodox here.

Fix:

  1. Materialize all slot in the chain of sourceExprs.
  2. Materialize agg info again after the creation of table ref with join operator
  3. Add all the SlotDescriptor to HashJoinNode's output from child output no matter is materilalized or not
  4. Once the Slot is required keep it in the output tuple instead of pruning it, no matter the slot is materialized or not

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 by explaining why you chose the solution you did and what alternatives you considered, etc...

@github-actions github-actions bot added area/planner Issues or PRs related to the query planner kind/test labels Aug 31, 2022
@Kikyou1997 Kikyou1997 force-pushed the fix_491 branch 2 times, most recently from 1617d3c to d065632 Compare August 31, 2022 12:27
@morrySnow morrySnow self-assigned this Aug 31, 2022
@Kikyou1997
Copy link
Contributor Author

@morrySnow @morrySnow

}
for (Expr expr : sourceExprs) {
if (!(expr instanceof SlotRef)) {
expr.materializeSrcExpr();
Copy link
Contributor

Choose a reason for hiding this comment

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

add comments

.stream()
.anyMatch(SlotDescriptor::isMaterialized)) {
aggregateInfo.materializeRequiredSlots(analyzer, null);
aggregateInfoList.forEach(a -> a.materializeRequiredSlots(analyzer, null));
Copy link
Contributor

Choose a reason for hiding this comment

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

add comments

@yiguolei yiguolei removed the dev/1.1.2 label Sep 2, 2022
@Kikyou1997 Kikyou1997 closed this Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/planner Issues or PRs related to the query planner kind/test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants