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

Support to_timestamp #838

Merged
merged 40 commits into from Nov 17, 2022
Merged

Support to_timestamp #838

merged 40 commits into from Nov 17, 2022

Conversation

sarahyurick
Copy link
Collaborator

Closes #831.

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2022

Codecov Report

Merging #838 (be05bac) into main (ab246b0) will increase coverage by 0.00%.
The diff coverage is 57.69%.

@@           Coverage Diff           @@
##             main     #838   +/-   ##
=======================================
  Coverage   75.50%   75.51%           
=======================================
  Files          73       73           
  Lines        3985     4011   +26     
  Branches      713      721    +8     
=======================================
+ Hits         3009     3029   +20     
+ Misses        818      817    -1     
- Partials      158      165    +7     
Impacted Files Coverage Δ
dask_sql/physical/rex/core/call.py 80.04% <57.69%> (-1.30%) ⬇️
dask_sql/_version.py 35.31% <0.00%> (+1.41%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

dask_planner/src/dialect.rs Outdated Show resolved Hide resolved
dask_planner/src/sql.rs Outdated Show resolved Hide resolved
dask_sql/physical/rex/core/call.py Outdated Show resolved Hide resolved
sarahyurick and others added 3 commits October 18, 2022 11:43
Co-authored-by: Charles Blackmon-Luca <20627856+charlesbluca@users.noreply.github.com>
Copy link
Collaborator

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

With #857 closed in favor of this, would you mind adding this parser.rs test in?

dask_planner/src/sql.rs Outdated Show resolved Hide resolved
@sarahyurick
Copy link
Collaborator Author

I'm not quite sure why test_fsql is consistently failing at https://github.com/dask-contrib/dask-sql/blob/main/tests/integration/test_fugue.py#L67

@ayushdg
Copy link
Collaborator

ayushdg commented Oct 20, 2022

I'm not quite sure why test_fsql is consistently failing at https://github.com/dask-contrib/dask-sql/blob/main/tests/integration/test_fugue.py#L67

I saw this pop up on a some tests unrelated to this pr: https://github.com/dask-contrib/dask-sql/actions/runs/3292477865/attempts/1.

They seem to be presenting only on Mac-os so far and seems intermittent. Might need to investigate further if this keeps showing up.

tests/integration/test_rex.py Outdated Show resolved Hide resolved
tests/integration/test_rex.py Outdated Show resolved Hide resolved
tests/integration/test_rex.py Outdated Show resolved Hide resolved
tests/integration/test_rex.py Show resolved Hide resolved
sarahyurick and others added 3 commits November 2, 2022 09:20
Copy link
Collaborator

@ayushdg ayushdg left a comment

Choose a reason for hiding this comment

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

Added some comments below.

Also looking through the postgresql docs the integer version of this method doesn't accept a format argument. Do you think it's possible to update the parser to handle this? If not we might need to either throw an error or ignore the format for integer cases.

Another note is that the format argument for postgresql expects a different formatting pattern than python's datetime. For now we should be good to use the python format but add that to the docs

dask_sql/physical/rex/core/call.py Outdated Show resolved Hide resolved
dask_sql/physical/rex/core/call.py Outdated Show resolved Hide resolved
dask_sql/physical/rex/core/call.py Outdated Show resolved Hide resolved
@sarahyurick
Copy link
Collaborator Author

sarahyurick commented Nov 2, 2022

Also looking through the postgresql docs the integer version of this method doesn't accept a format argument. Do you think it's possible to update the parser to handle this? If not we might need to either throw an error or ignore the format for integer cases.

Interesting, thanks! Yes, this should be easy to fix, so I can update this as well.

tests/integration/test_rex.py Outdated Show resolved Hide resolved
dask_sql/physical/rex/core/call.py Outdated Show resolved Hide resolved
dask_sql/physical/rex/core/call.py Outdated Show resolved Hide resolved
@ayushdg ayushdg merged commit 4933d34 into dask-contrib:main Nov 17, 2022
@sarahyurick sarahyurick deleted the to_timestamp branch May 26, 2023 22:15
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.

[ENH] Support for to_timestamp
4 participants