Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Enhancement](Nereids)Add nereids minidump #18747

Merged
merged 1 commit into from
May 11, 2023
Merged

[Enhancement](Nereids)Add nereids minidump #18747

merged 1 commit into from
May 11, 2023

Conversation

LiBinfeng-01
Copy link
Contributor

Add runnable mini dump demo in MinidumpTest

Problem summary

Describe your changes.

Checklist(Required)

  • Does it affect the original behavior
  • Has unit tests been added
  • Has document been added or modified
  • Does it need to update dependencies
  • Is this PR support rollback (If NO, please explain WHY)

Further comments

If this is a relatively large or complex change, kick off the discussion at [email protected] by explaining why you chose the solution you did and what alternatives you considered, etc...

@LiBinfeng-01
Copy link
Contributor Author

run build all

@LiBinfeng-01
Copy link
Contributor Author

run buildall

@morrySnow
Copy link
Contributor

run buildall

@LiBinfeng-01
Copy link
Contributor Author

run buildall

@hello-stephen
Copy link
Contributor

hello-stephen commented Apr 24, 2023 •
edited
Loading

TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 33.56 seconds
stream load tsv: 427 seconds loaded 74807831229 Bytes, about 167 MB/s
stream load json: 22 seconds loaded 2358488459 Bytes, about 102 MB/s
stream load orc: 60 seconds loaded 1101869774 Bytes, about 17 MB/s
stream load parquet: 31 seconds loaded 861443392 Bytes, about 26 MB/s
https://doris-community-test-1308700295.cos.ap-hongkong.myqcloud.com/tmp/20230510133955_clickbench_pr_141841.html

@LiBinfeng-01
Copy link
Contributor Author

run fest

@LiBinfeng-01
Copy link
Contributor Author

run feut

@LiBinfeng-01
Copy link
Contributor Author

run feut

@LiBinfeng-01
Copy link
Contributor Author

run buildall

@LiBinfeng-01
Copy link
Contributor Author

run feut

@LiBinfeng-01
Copy link
Contributor Author

run feut

@LiBinfeng-01
Copy link
Contributor Author

run buildall

jackwener
jackwener reviewed Apr 27, 2023
View reviewed changes
fe/fe-core/src/main/java/org/apache/doris/nereids/CascadesContext.java Outdated
@@ -332,6 +336,21 @@ public void extractTables(LogicalPlan logicalPlan) {
}
}

/** get table by table name, try to get from information from dumpfile first */
public Table getTableByName(String tableName) {
assert (tables != null);
Copy link
Member

Choose a reason for hiding this comment

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

use Precondition.checkState()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@LiBinfeng-01
Copy link
Contributor Author

run buildall

LiBinfeng-01
LiBinfeng-01 commented Apr 27, 2023
View reviewed changes
...e-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/AbstractPhysicalJoin.java
Comment on lines +188 to +189
@Override
public JSONObject toJson() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because abstractPhysicalJoin can be different with abstractLogicalJoin, so her can have some different implementation

jackwener
jackwener reviewed Apr 27, 2023
View reviewed changes
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalBinary.java Outdated
Comment on lines 50 to 60
@Override
public JSONObject toJson() {
JSONObject json = new JSONObject();
json.put("PlanType", getType().toString());
JSONArray childrenJson = new JSONArray();
for (Plan child : children) {
childrenJson.put(child.toJson());
}
json.put("children", childrenJson);
return json;
}
Copy link
Member

Choose a reason for hiding this comment

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

we don't implement this func for Unary Binary.
We can implement it for AbstractPlan, and check children().size is enough

Copy link
Contributor Author

@LiBinfeng-01 LiBinfeng-01 Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I considered this implementation when start doing this part, but found leafPlan do not have children member, so this can not be done

@LiBinfeng-01
Copy link
Contributor Author

run buildall

jackwener
jackwener reviewed Apr 27, 2023
View reviewed changes
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java Outdated
Comment on lines 945 to 948
public void setEnableNereidsTimeout(boolean enableNereidsTimeout) {
this.enableNereidsTimeout = enableNereidsTimeout;
}

Copy link
Member

Choose a reason for hiding this comment

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

Because variable is public, we don't need get/set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

jackwener
jackwener reviewed Apr 27, 2023
View reviewed changes
fe/fe-core/src/main/java/org/apache/doris/nereids/minidump/Minidump.java Outdated
Comment on lines 88 to 90
public Minidump() {

}
Copy link
Member

Choose a reason for hiding this comment

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

why add this a empty constructor, it's easy to use wrong by others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, it was added default by ide, I forgot to remove it

jackwener
jackwener reviewed May 10, 2023
View reviewed changes
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/rewrite/CustomRewriteJob.java Outdated
Comment on lines 48 to 51
public RuleType getRuleType() {
return ruleType;
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public RuleType getRuleType() {
return ruleType;
}

It's redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@github-actions github-actions bot removed the approved Indicates a PR has been approved by one committer. label May 10, 2023
@LiBinfeng-01
Copy link
Contributor Author

run buildall

@LiBinfeng-01
Copy link
Contributor Author

run buildall

1 similar comment
@LiBinfeng-01
Copy link
Contributor Author

run buildall

@LiBinfeng-01
Copy link
Contributor Author

run buildall

@LiBinfeng-01
Copy link
Contributor Author

run p0

@jackwener jackwener merged commit 99cef84 into apache:master May 11, 2023
morrySnow added a commit to morrySnow/incubator-doris that referenced this pull request May 29, 2024
@morrySnow
[fix](Nereids) remove getTableInMinidumpCache temporary
684e7d4 
bug introduced by apache#18747

getTableInMinidumpCache use wrong way to compare table's qualified name.
we remove it temporary since it not use in productive env anymore
@morrySnow morrySnow mentioned this pull request May 29, 2024
Merged
morrySnow added a commit to morrySnow/incubator-doris that referenced this pull request May 29, 2024
@morrySnow
[fix](Nereids) remove getTableInMinidumpCache temporary
11e139f 
pick from master apache#35571

bug introduced by apache#18747

getTableInMinidumpCache use wrong way to compare table's qualified name.
we remove it temporary since it not use in productive env anymore
@morrySnow morrySnow mentioned this pull request May 29, 2024
Merged
morrySnow added a commit to morrySnow/incubator-doris that referenced this pull request May 29, 2024
@morrySnow
[fix](Nereids) remove getTableInMinidumpCache temporary
d24cd9e 
bug introduced by apache#18747

getTableInMinidumpCache use wrong way to compare table's qualified name.
we remove it temporary since it not use in productive env anymore
morrySnow added a commit that referenced this pull request May 29, 2024
@morrySnow
[fix](Nereids) remove getTableInMinidumpCache temporary (#35578)
0443fc1 
pick from master #35571

bug introduced by #18747

getTableInMinidumpCache use wrong way to compare table's qualified name.
we remove it temporary since it not use in productive env anymore
morrySnow added a commit that referenced this pull request May 29, 2024
@morrySnow
[fix](Nereids) remove getTableInMinidumpCache temporary (#35571)
f6e624a 
bug introduced by #18747

getTableInMinidumpCache use wrong way to compare table's qualified name.
we remove it temporary since it not use in productive env anymore
yiguolei pushed a commit that referenced this pull request May 29, 2024
@morrySnow @Doris-Extras
[fix](Nereids) remove getTableInMinidumpCache temporary (#35571)
f6e8c53 
bug introduced by #18747

getTableInMinidumpCache use wrong way to compare table's qualified name.
we remove it temporary since it not use in productive env anymore
dataroaring pushed a commit that referenced this pull request May 31, 2024
@morrySnow @dataroaring
[fix](Nereids) remove getTableInMinidumpCache temporary (#35571)
6aff5b1 
bug introduced by #18747

getTableInMinidumpCache use wrong way to compare table's qualified name.
we remove it temporary since it not use in productive env anymore
mongo360 pushed a commit to mongo360/doris that referenced this pull request Aug 16, 2024
@morrySnow
[fix](Nereids) remove getTableInMinidumpCache temporary (apache#35578)
219a149 
pick from master apache#35571

bug introduced by apache#18747

getTableInMinidumpCache use wrong way to compare table's qualified name.
we remove it temporary since it not use in productive env anymore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackwener jackwener jackwener approved these changes

Assignees
No one assigned
Labels
area/nereids reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LiBinfeng-01 @morrySnow @hello-stephen @jackwener