Skip to content

Conversation

@jdye64
Copy link
Collaborator

@jdye64 jdye64 commented Jun 16, 2022

Closes: #463
Closes: #459

@jdye64 jdye64 added the datafusion Related to work in DataFusion label Jun 16, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2022

Codecov Report

❗ No coverage uploaded for pull request base (datafusion-sql-planner@453249e). Click here to learn what that means.
The diff coverage is n/a.

@@                    Coverage Diff                    @@
##             datafusion-sql-planner     #581   +/-   ##
=========================================================
  Coverage                          ?   60.80%           
=========================================================
  Files                             ?       72           
  Lines                             ?     3572           
  Branches                          ?      729           
=========================================================
  Hits                              ?     2172           
  Misses                            ?     1249           
  Partials                          ?      151           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@jdye64 jdye64 self-assigned this Jun 16, 2022
@jdye64 jdye64 marked this pull request as ready for review June 17, 2022 02:58
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.

Generally changes lgtm!

We need to add more testing around different date time arithmetic and operations. We can probably do it in a followup pr but should open an issue for tracking.

Especially for cases where sql interval types are being added/subtracted from dates/timestamps

galipremsagar
galipremsagar previously approved these changes Jun 21, 2022
@galipremsagar galipremsagar dismissed their stale review June 21, 2022 14:11

Seems like some pytest failures

Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Some comments/suggestions

@jdye64 jdye64 requested review from ayushdg and charlesbluca June 21, 2022 20:33
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.

Based on the discussion in #581 (comment) is it right to assume that this pr only handles datetime objects for now and a followup pr will be opened to handle #584?

@jdye64
Copy link
Collaborator Author

jdye64 commented Jun 22, 2022

Based on the discussion in #581 (comment) is it right to assume that this pr only handles datetime objects for now and a followup pr will be opened to handle #584?

Yes, that is correct

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.

One more comment but otherwise LGTM

@randerzander
Copy link
Collaborator

rerun tests

@charlesbluca charlesbluca merged commit cb1fe52 into dask-contrib:datafusion-sql-planner Jun 22, 2022
@jdye64 jdye64 deleted the datafusion-filter branch June 22, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

datafusion Related to work in DataFusion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants