-
Notifications
You must be signed in to change notification settings - Fork 114
Gold Standard: spark-only version for creating and comparing golden files #361
base: master
Are you sure you want to change the base?
Conversation
note to reviewers: There is one issue currently with this pr: when running an individual query against approved files, the tests fail. When running all queries at the same time, the tests pass. I haven't yet fixed this issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apoorvedave1 Could you create a PR with just *.scala and one query set?
|
Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update PR description with what each directory contains and where you got them from?
- src/test/resources/tpcds-plan-stability/approved-plans-v1_4/ (also, simplified.txt vs. explain.txt)
- src/test/resources/tpcds/
Also, update it with how you generated them (mostly you can copy the instruction from PlanStabilitySuite.scala?
import org.apache.spark.sql.execution.command.ExplainCommand | ||
import org.apache.spark.sql.execution.exchange.{Exchange, ReusedExchangeExec} | ||
|
||
// scalastyle:off filelinelengthchecker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is taken from OSS Spark, can you have a link to credit it correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, can you add guided comments if some portions of this file are modified for Hyperspace to accelerate the review process? (I don't need to review that's already from OSS Spark)
|
||
import com.microsoft.hyperspace.SparkInvolvedSuite | ||
|
||
trait TPCDSBase extends SparkFunSuite with SparkInvolvedSuite { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto for this file (if applicable)
"q12", "q13", "q14a", "q14b", "q15", "q16", "q17", "q18", "q19", "q20", | ||
"q21", "q22", "q23a", "q23b", "q24a", "q24b", "q25", "q26", "q27", "q28", "q29", "q30", | ||
"q31", "q32", "q33", "q34", "q35", "q36", "q37", "q38", "q39a", "q39b", "q40", | ||
"q41", "q42", "q43", "q44", "q45", "q46", "q47", "q48", "q49", "q50", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need to just take out q49 here? If this is possible, you can add a TODO and link to an issue.
What is the context for this pull request?
Gold standard for non-hyperspace plan stability suite
Parent Issue: #334
What changes were proposed in this pull request?
Gold standard for non-hyperspace plan stability suite (vanilla spark version)
Does this PR introduce any user-facing change?
no
How was this patch tested?
unit tests