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

[GLUTEN-6827][VL] Add a new test case for Round's coverage #6884

Merged
merged 1 commit into from
Aug 19, 2024
Merged

[GLUTEN-6827][VL] Add a new test case for Round's coverage #6884

merged 1 commit into from
Aug 19, 2024

Conversation

jiangjiangtian
Copy link
Contributor

A new test case found in #6827.
After fix, we can uncomment it.

@github-actions github-actions bot added the CORE works for Gluten Core label Aug 16, 2024
@github-actions GitHub Actions
Copy link

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

@jiangjiangtian
Copy link
Contributor Author

cc @zhztheplayer

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@zhztheplayer
Copy link
Member

Hi @jiangjiangtian, would you think it's possible to add a case to TestOperator.scala as well? Which is using SQL and have result comparison with vanilla Spark in it. So we make it's clear that Gluten gives different answer for the case.

@zhouyuan zhouyuan changed the title [VL] add a new test case for Round's coverage [GLUTEN-6827][VL] add a new test case for Round's coverage Aug 16, 2024
@github-actions GitHub Actions
Copy link

#6827

@github-actions github-actions bot added the VELOX label Aug 16, 2024
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@jiangjiangtian
Copy link
Contributor Author

Hi @jiangjiangtian, would you think it's possible to add a case to TestOperator.scala as well? Which is using SQL and have result comparison with vanilla Spark in it. So we make it's clear that Gluten gives different answer for the case.

@zhztheplayer Thank for your remind. I have add a test in TestOperator.scala. Please review.

@zhztheplayer zhztheplayer changed the title [GLUTEN-6827][VL] add a new test case for Round's coverage [GLUTEN-6827][VL] Add a new test case for Round's coverage Aug 16, 2024
zhztheplayer
zhztheplayer reviewed Aug 16, 2024
View reviewed changes
backends-velox/src/test/scala/org/apache/gluten/execution/TestOperator.scala Outdated
Comment on lines 2083 to 2088
// test("Test round expression") {
// val df1 = runQueryAndCompare("SELECT round(cast(0.5549999999999999 as double), 2)") { _ => }
// checkLengthAndPlan(df1, 1)
// val df2 = runQueryAndCompare("SELECT round(cast(0.19324999999999998 as double), 2)") { _ => }
// checkLengthAndPlan(df2, 1)
// }
Copy link
Member

Choose a reason for hiding this comment

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

Comparing to commenting the code out, you can use ignore scalatest flag, e.g.,

ignore("Test round expression") {
  ...
}

zhztheplayer
zhztheplayer reviewed Aug 16, 2024
View reviewed changes
backends-velox/src/test/scala/org/apache/gluten/execution/TestOperator.scala Outdated
@@ -2078,4 +2078,12 @@ class TestOperator extends VeloxWholeStageTransformerSuite with AdaptiveSparkPla
checkGlutenOperatorMatch[SortExecTransformer]
}
}

// Open it after fix
Copy link
Member

Choose a reason for hiding this comment

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

Better change to // Enable the test after fixing https://github.com/apache/incubator-gluten/issues/6827, thanks.

zhztheplayer
zhztheplayer reviewed Aug 16, 2024
View reviewed changes
...32/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala Outdated
@@ -124,6 +124,8 @@ class GlutenMathExpressionsSuite extends MathExpressionsSuite with GlutenTestsTr
checkEvaluation(Round(1.12345678901234567, 8), 1.12345679)
checkEvaluation(Round(-0.98765432109876543, 5), -0.98765)
checkEvaluation(Round(12345.67890123456789, 6), 12345.678901)
// Open it after fix
Copy link
Member

Choose a reason for hiding this comment

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

ditto

zhztheplayer
zhztheplayer reviewed Aug 16, 2024
View reviewed changes
...33/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala Outdated
@@ -252,6 +252,8 @@ class GlutenMathExpressionsSuite extends MathExpressionsSuite with GlutenTestsTr
checkEvaluation(Round(1.12345678901234567, 8), 1.12345679)
checkEvaluation(Round(-0.98765432109876543, 5), -0.98765)
checkEvaluation(Round(12345.67890123456789, 6), 12345.678901)
// Open it after fix
Copy link
Member

Choose a reason for hiding this comment

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

ditto

zhztheplayer
zhztheplayer reviewed Aug 16, 2024
View reviewed changes
...34/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala Outdated
@@ -248,6 +248,8 @@ class GlutenMathExpressionsSuite extends MathExpressionsSuite with GlutenTestsTr
checkEvaluation(BRound(-3.5, 0), -4.0)
checkEvaluation(BRound(-0.35, 1), -0.4)
checkEvaluation(BRound(-35, -1), -40)
// Open it after fix
Copy link
Member

Choose a reason for hiding this comment

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

ditto

zhztheplayer
zhztheplayer reviewed Aug 16, 2024
View reviewed changes
...35/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala Outdated
@@ -251,6 +251,8 @@ class GlutenMathExpressionsSuite extends MathExpressionsSuite with GlutenTestsTr
checkEvaluation(Round(1.12345678901234567, 8), 1.12345679)
checkEvaluation(Round(-0.98765432109876543, 5), -0.98765)
checkEvaluation(Round(12345.67890123456789, 6), 12345.678901)
// Open it after fix
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

zhztheplayer
zhztheplayer previously approved these changes Aug 16, 2024
View reviewed changes
Copy link
Member

@zhztheplayer zhztheplayer left a comment

Choose a reason for hiding this comment

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

👍

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@jiangjiangtian
Copy link
Contributor Author

@zhztheplayer Sorry, I forget to add a comment and I add it just now. Please review again.

zhztheplayer
zhztheplayer approved these changes Aug 16, 2024
View reviewed changes
Copy link
Member

@zhztheplayer zhztheplayer left a comment

Choose a reason for hiding this comment

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

Thanks!

@zhztheplayer zhztheplayer merged commit 4f7ac37 into apache:main Aug 19, 2024
43 checks passed
@jiangjiangtian jiangjiangtian deleted the round_test branch August 19, 2024 01:58
sharkdtu pushed a commit to sharkdtu/gluten that referenced this pull request Nov 11, 2024
@jiangjiangtian
[VL] Add a new test case for Round's coverage (apache#6884)
8ded15c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhztheplayer zhztheplayer zhztheplayer approved these changes

Assignees
No one assigned
Labels
CORE works for Gluten Core VELOX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jiangjiangtian @zhztheplayer