-
Notifications
You must be signed in to change notification settings - Fork 458
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.
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
[GLUTEN-4803][UT] Add Golden Files for TPC-H Spark33 + Gluten Execution Plan #4804
Conversation
Run Gluten Clickhouse CI |
1 similar comment
Run Gluten Clickhouse CI |
Hi @ulysses-you @PHILO-HE, please take a review and help re-trigger the fail ci. |
.github/workflows/velox_be.yml
Outdated
- name: Upload golden files | ||
if: failure() | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: golden-files-spark32 | ||
path: | | ||
/tmp/${{ github.run_id }}/tpch-approved-plan/** | ||
/tmp/${{ github.run_id }}/spark322/tpch-approved-plan/** |
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.
does the path correct ?
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.
Yes, this path is CI Local Disk Path, which will be created when copy golden files from docker image.
@@ -81,7 +81,7 @@ abstract class VeloxTPCHSuite extends VeloxTPCHTableSupport { | |||
def shouldCheckGoldenFiles(): Boolean = { | |||
Seq("v1", "v1-bhj").contains(subType()) && (formatSparkVersion match { | |||
case "322" => true | |||
case "331" => false | |||
case "331" => true |
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.
can we use the major version, 32
, 33
? what happens if we update Spark minor version
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.
Sound good to me, we can match those main version with startsWith
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.
Update, please take a review.
Run Gluten Clickhouse CI |
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.
the golden files path did not change ..
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.
Well, right, in order to avoid frequent changes caused by upgrading the minor version, the concept of the minor version should be masked
Run Gluten Clickhouse CI |
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.
Looks good! Thanks!
- name: Upload golden files | ||
if: failure() | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: golden-files-spark32 | ||
path: | | ||
/tmp/${{ github.run_id }}/tpch-approved-plan/** | ||
/tmp/${{ github.run_id }}/spark32/tpch-approved-plan/** |
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.
Nit: can we use $GITHUB_RUN_ID for consistency?
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.
I tried using $GITHUB_RUN_ID
, but it is considered as string not a var. Then take this approach.
let's hold on this pr for #4815 |
Run Gluten Clickhouse CI |
Do some rebase operations, mark as draft for now. |
Run Gluten Clickhouse CI |
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.
Thanks for your continuous efforts!
What changes were proposed in this pull request?
Close #4803
As title, follow up to add TPC-H + Spark33 Golden Files.
How was this patch tested?
unit tests