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.

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

minor: cast timestamp test #468 #923

Merged
merged 1 commit into from
Sep 11, 2024
Merged

minor: cast timestamp test #468 #923

merged 1 commit into from
Sep 11, 2024

Conversation

himadripal
Copy link
Contributor

@himadripal himadripal commented Sep 7, 2024

Removed assume(isspark34Plus).
Tested with `make test PROFILES="-Pspark-3.3".
3.2 is not supported.
All worked.

Closes #468

@himadripal
Copy link
Contributor Author

himadripal commented Sep 7, 2024

@viirya @andygrove @parthchandra , please take a look.

@comphead
Copy link
Contributor

comphead commented Sep 7, 2024

Thanks @himadripal for your contribution, triggering the workflow

@comphead
Copy link
Contributor

comphead commented Sep 7, 2024

Please fix the PR title,

Please use a title in the format: type: message, or type(scope): message
Example: feat: Add support for sort-merge join

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.92%. Comparing base (033fe6f) to head (6534a9d).
Report is 13 commits behind head on main.

Additional details and impacted files
@@              Coverage Diff              @@
##               main     #923       +/-   ##
=============================================
+ Coverage     34.03%   54.92%   +20.88%     
+ Complexity      883      853       -30     
=============================================
  Files           113      109        -4     
  Lines         43170    10697    -32473     
  Branches       9516     2050     -7466     
=============================================
- Hits          14693     5875     -8818     
+ Misses        25471     3789    -21682     
+ Partials       3006     1033     -1973     

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

@himadripal himadripal changed the title Fix cast timestamp test #468 feat: cast timestamp test #468 Sep 8, 2024
@himadripal himadripal changed the title feat: cast timestamp test #468 minor: cast timestamp test #468 Sep 9, 2024
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @himadripal
@andygrove please have a look as well, as you originated this ticket

Copy link
Contributor

@parthchandra parthchandra left a comment

Choose a reason for hiding this comment

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

Afaik, these tests were being run only for Spar k 3.4 because Spark had incorrect output for these cases in 3.2 and we could not make these tests pass. If these tests pass for Spark 3.3, we can remove this check.

@viirya
Copy link
Member

viirya commented Sep 9, 2024

The issue claims that the test fails to pass for Spark 3.2 and 3.3. We don't have 3.2 test but 3.3 is still there. Any change that makes it passed in Spark 3.3 now?

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @himadripal

When I filed the issue it looks like I only tested with Spark 3.2 and did not test with 3.3, so I am not sure if there was really an issue with 3.3 at that time or not.

@comphead comphead merged commit 165fd43 into apache:main Sep 11, 2024
75 of 77 checks passed
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.

bug: CAST timestamp to string ignores timezone prior to Spark 3.4
6 participants