Skip to content

Conversation

@morrySnow
Copy link
Contributor

@morrySnow morrySnow commented May 23, 2022

Proposed changes

Issue Number: close #9740

Problem Summary:

Current, we process order by and limit in Doris only way:
https://github.com/apache/incubator-doris/blob/d97e2b1eb21b1026ef95f0cb5b87e0138c704de8/fe/fe-core/src/main/cup/sql_parser.cup#L3174-L3191

We need to change it behavior to standard SQL. query organization need to be executed at last by default.

This PR changes syntax to match standard sql:

query = ctes? set_operand_list order_by limit
set_operand_list = set_operand | set_operand_list set_op set_operand
set_operand = selectClause fromClause? whereClause? aggregationClause? havingClause?
    | values
    | LEFT_PAREN query RIGHT_PAREN

After applied this PR, All query organization will be executed at last. If you want it only affect on the last operand in operand set, you need to add parentheses on the last operand.

For example:

case 1, query organization only apply on last operand:

-- original behavior
SELECT * FROM t1 UNION ALL SELECT * FROM t2 ORDER BY col1;

-- new behavior
SELECT * FROM t1 UNION ALL (SELECT * FROM t2 ORDER BY col1);

case 2, query organization only apply on global:

-- original behavior
(SELECT * FROM t1 UNION ALL SELECT * FROM t2) ORDER BY col1;

-- new behavior
SELECT * FROM t1 UNION ALL SELECT * FROM t2 ORDER BY col1;

Checklist(Required)

  1. Does it affect the original behavior: (Yes)
  2. Has unit tests been added: (Yes)
  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

add unit test and regression test later

@github-actions github-actions bot added the area/planner Issues or PRs related to the query planner label May 23, 2022
@morrySnow morrySnow added dev/1.0.1-deprecated should be merged into dev-1.0.1 branch kind/behavior-changed labels May 23, 2022
@morrySnow morrySnow requested a review from morningman May 24, 2022 10:46
@morningman morningman added this to the v1.2 milestone May 27, 2022
@morningman morningman removed the dev/1.0.1-deprecated should be merged into dev-1.0.1 branch label May 27, 2022
kpfly
kpfly previously approved these changes Jul 18, 2022
Copy link

@kpfly kpfly left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@924060929
Copy link
Contributor

please add a ut that will throw exception when exist order/limit clause in select statement with union

select id from a limit 10 union select id from b

@morrySnow morrySnow changed the title [enhancement] (planner) change Doris's query organization syntax to standard sql [behavior change](planner)change Doris's query organization syntax to standard sql Aug 25, 2022
Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

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

LGTM

@morningman morningman merged commit f19c344 into apache:master Aug 25, 2022
@morrySnow morrySnow deleted the limit_set_order branch August 25, 2022 09:20
morningman added a commit that referenced this pull request Aug 29, 2022
morningman added a commit that referenced this pull request Aug 29, 2022
…yntax to standard sql (#9745)" (#12135)

Reverts #9745
This may cause NPE when calling `parseDefineExprWithoutAnalyze()`
GoGoWen pushed a commit to GoGoWen/incubator-doris that referenced this pull request Aug 31, 2022
…yntax to standard sql (apache#9745)" (apache#12135)

Reverts apache#9745
This may cause NPE when calling `parseDefineExprWithoutAnalyze()`
Yukang-Lian pushed a commit to Yukang-Lian/doris that referenced this pull request Sep 2, 2022
…yntax to standard sql (apache#9745)" (apache#12135)

Reverts apache#9745
This may cause NPE when calling `parseDefineExprWithoutAnalyze()`
Yukang-Lian pushed a commit to Yukang-Lian/doris that referenced this pull request Sep 2, 2022
…yntax to standard sql (apache#9745)" (apache#12135)

Reverts apache#9745
This may cause NPE when calling `parseDefineExprWithoutAnalyze()`
@morningman morningman mentioned this pull request Nov 21, 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/behavior-changed reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] (planner) change Doris's query organization syntax to standard sql

4 participants