tests: Move random data generation methods from CometCastSuite to new DataGenerator class #426

andygrove · 2024-05-14T15:19:06Z

Which issue does this PR close?

N/A

Rationale for this change

Start moving random data generation out of CometCastSuite and into a class that can be used from other test suites.

What changes are included in this PR?

Refactor data generator
Mark CAST string -> integer as incompatible because the data gen changes exposed new issues

How are these changes tested?

Existing tests

andygrove · 2024-05-14T15:20:09Z

spark/src/test/scala/org/apache/comet/CometCastSuite.scala

@@ -907,28 +866,6 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
    }
  }

-  // TODO Commented out to work around scalafix since this is currently unused.


Removing this commented out code since it is no longer needed

advancedxy

The changes looks good to me except one minor comment.

However, the CI is failing, you may need to take a look at that.

advancedxy · 2024-05-14T16:18:36Z

spark/src/test/scala/org/apache/comet/CometCastSuite.scala

@@ -35,6 +35,9 @@ import org.apache.comet.expressions.{CometCast, Compatible}
 class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
  import testImplicits._

+  /** Create a data generator using a fixed seed so that tests are reproducible */
+  private val gen = new DataGenerator(new Random(42))


I think it would be better to make this instance a field in the Companion object of ~~Random~~DataGenerator?

So that other test suites can all use the same random data generator by default.

To make sure I understand what you are suggesting, I think you are saying to add this:

object DataGenerator { val DEFAULT = new DataGenerator(new Random(42)) }

And then reference that from the test suite?

private val gen = DataGenerator.DEFAULT

I think this could lead to some non-deterministic behavior. For example, running CometCastSuite on its own from my IDE would start with the freshly created generator, but running the same suite after after test suites have run would result in different inputs.

I think it would be better for each test to create a new instance of the generator with a fixed seed?

I suppose we could define it as a def rather than a val to force a new creation each time. WDYT?

Thanks for the new suggested approach, that’s better and should be used.

Thanks for the review @advancedxy. I have implemented your suggestion.

andygrove · 2024-05-14T16:27:46Z

However, the CI is failing, you may need to take a look at that.

Yes, this change effectivey changed the seed and it has exposed new compatibility issues in existing tests. I am investigating.

codecov-commenter · 2024-05-14T18:36:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.18%. Comparing base (14494d3) to head (cf779df).
Report is 9 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #426      +/-   ##
============================================
+ Coverage     34.02%   34.18%   +0.16%     
+ Complexity      857      850       -7     
============================================
  Files           116      116              
  Lines         38565    38552      -13     
  Branches       8517     8523       +6     
============================================
+ Hits          13120    13178      +58     
+ Misses        22691    22609      -82     
- Partials       2754     2765      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andygrove · 2024-05-14T19:29:21Z

spark/src/main/scala/org/apache/comet/expressions/CometCast.scala

@@ -108,7 +108,7 @@ object CometCast {
        Compatible()
      case DataTypes.ByteType | DataTypes.ShortType | DataTypes.IntegerType |
          DataTypes.LongType =>
-        Compatible()
+        Incompatible(Some("Not all invalid inputs are detected"))


I filed #431 to fix this

kazuyukitanimura · 2024-05-14T23:51:23Z

spark/src/test/scala/org/apache/comet/CometCastSuite.scala

+  private val gen = DataGenerator.DEFAULT
+
+  /** Number of random data items to generate in each test */
+  private val dataSize = 10000


So the size is increased 10x?

Yes. At 1000 I saw string -> short fail, but not the other string -> integer tests. When I increased this to 10000 then all of the string -> integer tests failed, so seems like we get better coverage at this level

kazuyukitanimura · 2024-05-15T00:00:45Z

spark/src/test/scala/org/apache/comet/DataGenerator.scala

+      Double.NaN,
+      Double.PositiveInfinity,
+      Double.NegativeInfinity,
+      0.0d) ++


Should we cover

1.0d, -1.0d, Int.MinValue.toDouble, Int.MaxValue.toDouble, 0.0d, -0.0d

?

+1. But I think it could be addressed in a follow-up pr.

advancedxy · 2024-05-15T01:09:42Z

spark/src/test/scala/org/apache/comet/DataGenerator.scala

+      Double.NaN,
+      Double.PositiveInfinity,
+      Double.NegativeInfinity,
+      0.0d) ++


+1. But I think it could be addressed in a follow-up pr.

viirya · 2024-05-15T18:54:38Z

Merged. Thanks @andygrove

… DataGenerator class (apache#426) * add new DataGenerator class * move more data gen methods * ignore some failing tests, update compat docs * address feedback * fix regression (cherry picked from commit fcf7d5b)

andygrove added 2 commits May 14, 2024 09:03

add new DataGenerator class

892d66d

move more data gen methods

23082da

andygrove commented May 14, 2024

View reviewed changes

andygrove mentioned this pull request May 14, 2024

feat: Add xxhash64 function support #424

Merged

advancedxy reviewed May 14, 2024

View reviewed changes

andygrove added 2 commits May 14, 2024 10:50

ignore some failing tests, update compat docs

b109871

address feedback

edf28d9

andygrove requested a review from viirya May 14, 2024 17:01

fix regression

cf779df

andygrove mentioned this pull request May 14, 2024

bug: CAST string to integer does not handle all invalid inputs #431

Closed

andygrove commented May 14, 2024

View reviewed changes

kazuyukitanimura approved these changes May 15, 2024

View reviewed changes

advancedxy approved these changes May 15, 2024

View reviewed changes

advancedxy mentioned this pull request May 15, 2024

fix: Compute murmur3 hash with dictionary input correctly #433

Merged

viirya approved these changes May 15, 2024

View reviewed changes

viirya merged commit fcf7d5b into apache:main May 15, 2024
40 checks passed

andygrove deleted the datagen branch May 15, 2024 19:01

advancedxy mentioned this pull request May 20, 2024

feat: Add random row generator in data generator #451

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: Move random data generation methods from CometCastSuite to new DataGenerator class #426

tests: Move random data generation methods from CometCastSuite to new DataGenerator class #426

andygrove commented May 14, 2024 •

edited

Loading

andygrove May 14, 2024

advancedxy left a comment

advancedxy May 14, 2024

andygrove May 14, 2024

andygrove May 14, 2024

advancedxy May 14, 2024

andygrove May 14, 2024

andygrove commented May 14, 2024

codecov-commenter commented May 14, 2024

andygrove May 14, 2024

kazuyukitanimura May 14, 2024

andygrove May 15, 2024

kazuyukitanimura May 15, 2024

advancedxy May 15, 2024

advancedxy May 15, 2024

viirya commented May 15, 2024

tests: Move random data generation methods from CometCastSuite to new DataGenerator class #426

tests: Move random data generation methods from CometCastSuite to new DataGenerator class #426

Conversation

andygrove commented May 14, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Choose a reason for hiding this comment

advancedxy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove commented May 14, 2024

codecov-commenter commented May 14, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented May 15, 2024

andygrove commented May 14, 2024 •

edited

Loading