Datafusion expand scalarvalue catchall #638
Conversation
…atafusion-sql-planner
…atafusion-sql-planner
…atafusion-sql-planner
…atafusion-sql-planner
…atafusion-sql-planner
…atafusion-sql-planner
| literal_value = None | ||
| elif literal_type == "IntervalDayTime": | ||
| literal_type = SqlTypeName.INTERVAL | ||
| literal_value = rex.getIntervalDayTimeValue() |
There was a problem hiding this comment.
What kind of literal_value is returned in this case? Is it a string or an integer?
There was a problem hiding this comment.
In that case we might have to update the logic in sql to python value, since the conversion there seems to assume it's a string and do some computation instead.
There was a problem hiding this comment.
I think int should be fine as long as we update the logic here as well: https://github.com/dask-contrib/dask-sql/blob/datafusion-sql-planner/dask_sql/mappings.py#L131.
I'm surprised all tests passed. Maybe none actually test the case where an interval type is passed?
|
Would it be helpful if I made it return a String instead? That is possible with a little wrapper work
Get Outlook for iOS<https://aka.ms/o0ukef>
________________________________
From: Ayush Dattagupta ***@***.***>
Sent: Thursday, July 21, 2022 4:05:03 PM
To: dask-contrib/dask-sql ***@***.***>
Cc: Jeremy Dyer ***@***.***>; Assign ***@***.***>
Subject: Re: [dask-contrib/dask-sql] Datafusion expand scalarvalue catchall (PR #638)
@ayushdg commented on this pull request.
________________________________
In dask_sql/physical/rex/core/literal.py<#638 (comment)>:
@@ -145,8 +145,13 @@ def convert(
elif literal_type == "Null":
literal_type = SqlTypeName.NULL
literal_value = None
+ elif literal_type == "IntervalDayTime":
+ literal_type = SqlTypeName.INTERVAL
+ literal_value = rex.getIntervalDayTimeValue()
In that case we might have to update the logic in sql to python value, since the conversion there seems to assume it's a string and do some computation instead.
—
Reply to this email directly, view it on GitHub<#638 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAQHLAYRG5BGS5OYI6G6KGLVVGUO7ANCNFSM54EB4U7Q>.
You are receiving this because you were assigned.Message ID: ***@***.***>
|
|
@ayushdg can we merge this one now? |
Could we add some tests around interval types or move the interval handling to a followup pr if the other functionality is needed. There's still some issues to iron out there, and statements like |
…ed by Rust, first one for days, second one for millisecond in incomplete day
Codecov Report
@@ Coverage Diff @@
## datafusion-sql-planner #638 +/- ##
=========================================================
Coverage ? 65.89%
=========================================================
Files ? 73
Lines ? 3633
Branches ? 752
=========================================================
Hits ? 2394
Misses ? 1091
Partials ? 148 📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
@ayushdg the failures are unrelated and are part of codecov issues that @charlesbluca is working on I. #666. Does this PR look good to you know with the changes? |
ayushdg
left a comment
ayushdg
left a comment
There was a problem hiding this comment.
Changes lgtm! The other interval types can be added in a followup pr.
| } | ||
| } | ||
|
|
||
| #[pyo3(name = "getIntervalDayTimeValue")] |
There was a problem hiding this comment.
We should also add methods to extract this information from interval yearMonth and MonthDayNano types
|
|
||
| elif sql_type == SqlTypeName.INTERVAL_DAY: | ||
| return timedelta(days=literal_value[0], milliseconds=literal_value[1]) | ||
| elif sql_type == SqlTypeName.INTERVAL: |
3 participants
We were previously "catching" all encountered
ScalarValuevariants in a single arm and panic-ing. This is the first step in not doing that.