Skip to content

Conversation

@andygrove
Copy link
Member

Which issue does this PR close?

Part of #286

Rationale for this change

What changes are included in this PR?

  • Improved CometCastSuite to assert that cast tests are enabled (and therefore passing) for casts that we report as compatible. This helps ensure that the docs are in sync with the real status. There is likely more that we can do in the future but this is another step closer.
  • Update compatibility reporting based on errors found by this new test enhancement
  • Added some notes on some of the current incompatibilities so that they are in the documentation
  • Improvements to the documentation formatting
  • Fixed a regression from a recent PR where the cast float/double to string tests weren't actually running the test

Docs Preview

We have a table of compatible casts, followed by:

Screenshot 2024-05-03 at 11 30 15 PM

How are these changes tested?

Tested docs locally.

0.0f) ++
Range(0, dataSize).map(_ => r.nextFloat())
withNulls(values).toDF("a")
castTest(withNulls(values).toDF("a"), DataTypes.StringType)
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing a regression that was introduced in #346

0.0d) ++
Range(0, dataSize).map(_ => r.nextDouble())
withNulls(values).toDF("a")
castTest(withNulls(values).toDF("a"), DataTypes.StringType)
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing a regression that was introduced in #346

"2024-01-01T12:34:56.123456",
"2024-01-01T01:00:00Z",
"2024-12-31T01:00:00-02:00",
"9999-12-31T01:00:00-02:00",
Copy link
Member Author

Choose a reason for hiding this comment

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

Just making sure we have a wider range of years being tested

if (line.trim == "<!--CAST_TABLE-->") {
w.write("| From Type | To Type | Compatible? | Notes |\n".getBytes)
w.write("|-|-|-|-|\n".getBytes)
if (line.trim == "<!--COMPAT_CAST_TABLE-->") {
Copy link
Member Author

Choose a reason for hiding this comment

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

There was previously one table, but now we show separate tables for compatible and incompatible.

CometConf.COMET_EXEC_SHUFFLE_ENABLED.key -> "true",
CometConf.COMET_COLUMNAR_SHUFFLE_ENABLED.key -> "true") {
CometConf.COMET_COLUMNAR_SHUFFLE_ENABLED.key -> "true",
CometConf.COMET_CAST_ALLOW_INCOMPATIBLE.key -> "true") {
Copy link
Member Author

Choose a reason for hiding this comment

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

This test needs binary -> string casts

@andygrove andygrove requested review from sunchao and viirya May 6, 2024 14:29
@andygrove
Copy link
Member Author

@viirya @snmvaughan This PR incorporates some of the feedback from #362

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @andygrove

@andygrove andygrove merged commit 8e73f7c into apache:main May 7, 2024
himadripal pushed a commit to himadripal/datafusion-comet that referenced this pull request Sep 7, 2024
@andygrove andygrove deleted the cast-compat-b branch December 3, 2024 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants