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

add error section in report and the rest queries #9150

Merged
merged 6 commits into from
Sep 7, 2023
Merged

add error section in report and the rest queries #9150

merged 6 commits into from
Sep 7, 2023

Conversation

wjxiz1992
Copy link
Collaborator

@wjxiz1992 wjxiz1992 commented Aug 31, 2023 •
edited
Loading

Close #8816 and #9118
Close #9165 and #9166
This PR adds in

  • the query content, an error field in the json report file to see what exceptions are thrown during the query run.
    Not only the exceptions that a failed query throws, but also the exceptions that a Spark task failed at first but succeeded in retry. ([FEA] add error section in test report json file #9118)
  • the rest queries for Scale Test.([FEA] Write the rest of the scale test queries #8816 )
  • a new input option --dry: will only print the generated queries and its physical plan(.exlpain())
  • add correct candidate columns for key group 2

This PR is still in testing with different scales, but it contains lots of queries, and it requires a lot of review for all of them since they are parsed according to my personal understanding. Put it up for early feedbacks.

put one generated queries for reviewers to help easy check the query correctness or if the queries are expected:
queries.txt

@wjxiz1992 wjxiz1992 self-assigned this Aug 31, 2023
@wjxiz1992 wjxiz1992 requested a review from revans2 August 31, 2023 11:15
andygrove
andygrove reviewed Aug 31, 2023
View reviewed changes
...ation_tests/src/main/scala/com/nvidia/spark/rapids/tests/scaletest/TaskFailureListener.scala
@@ -0,0 +1,24 @@
package com.nvidia.spark.rapids.tests.scaletest
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing license header

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added.

andygrove
andygrove reviewed Aug 31, 2023
View reviewed changes
integration_tests/src/main/scala/com/nvidia/spark/rapids/tests/scaletest/Utils.scala
@@ -0,0 +1,13 @@
package com.nvidia.spark.rapids.tests.scaletest
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing license header

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added.

@wjxiz1992
Copy link
Collaborator Author

wjxiz1992 commented Sep 1, 2023 •
edited
Loading

Found one blocking bug: #9165.
And another bug is not blocking but better to fix in this PR as well: #9166

convert it to draft until I resolve the 2 bugs above.

@wjxiz1992 wjxiz1992 marked this pull request as draft September 1, 2023 08:39
@wjxiz1992 wjxiz1992 marked this pull request as ready for review September 4, 2023 10:07
@wjxiz1992
Copy link
Collaborator Author

build

1 similar comment
@firestarman
Copy link
Collaborator

build

@wjxiz1992
Copy link
Collaborator Author

Hi @revans2 I've added all queries into the test suite, please help take a look when you have time. I've also put a query content list queries.txt in the attachment in the PR description.
Let me know if the queries are not generated as expected.

revans2
revans2 previously approved these changes Sep 6, 2023
View reviewed changes
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

In general it looks good, but I want to spend some time to test it myself. Just to be sure that the scale_factor/etc look correct and that the queries tax the plugin in ways we expect it to.

integration_tests/src/main/scala/com/nvidia/spark/rapids/tests/scaletest/QuerySpecs.scala Outdated
@@ -61,14 +243,14 @@ class QuerySpecs(config: Config, spark: SparkSession) {
}
}

def getCandidateQueries(): Map[String, TestQuery] = {
def getCandidateQueries(): mutable.LinkedHashMap[String, TestQuery] = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are the maps mutable? Do we plan on changing them after they are constructed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was looking for Map that can reserve insertion order and found LinkedHashMap first. I just found ListMap can also do this, updated.

integration_tests/src/main/scala/com/nvidia/spark/rapids/tests/scaletest/QuerySpecs.scala Outdated
config.iterations,
config.timeout,
"No obvious build side inner equi-join. (Shuffle partitions should be set to 10)",
shufflePartitions = 10),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why 10 shuffle partitions? The original proposal was ceil(scale_factor/50). Not that this is wrong, just curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I was refering to the queries in #8816. Now updated to what is set in test plan doc.

@wjxiz1992
resolve nits
21a1fda 
Signed-off-by: Allen Xu <[email protected]>
@wjxiz1992
Copy link
Collaborator Author

build

1 similar comment
@wjxiz1992
Copy link
Collaborator Author

build

@wjxiz1992
Copy link
Collaborator Author

In general it looks good, but I want to spend some time to test it myself. Just to be sure that the scale_factor/etc look correct and that the queries tax the plugin in ways we expect it to.

I can build pipelines to test this, any test preference? According to the test plan doc, I should start to generate scale_factor=100, complexity=300 data. I don't have enough disk space in my local PC so I can do this on our internal cluster.

@revans2
Copy link
Collaborator

revans2 commented Sep 7, 2023

@wjxiz1992 lets check this in and then we can work on figuring out if we need to tweak things. We need to setup a pipeline anyways, so lets get started with that as a separate piece.

@revans2 revans2 merged commit e35e194 into NVIDIA:branch-23.10 Sep 7, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andygrove andygrove andygrove left review comments

@revans2 revans2 revans2 approved these changes

Assignees

@wjxiz1992 wjxiz1992

Labels
data gen scale test test Only impacts tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Data gen for key groups produces type-mismatch columns [FEA] Write the rest of the scale test queries
4 participants
@wjxiz1992 @firestarman @revans2 @andygrove