-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add support for CREATE EXPERIMENT
, expand support for WITH
kwargs
#796
Add support for CREATE EXPERIMENT
, expand support for WITH
kwargs
#796
Conversation
dask_planner/src/parser.rs
Outdated
Value::Boolean(value) => Ok(if *value { | ||
"1".to_string() | ||
} else { | ||
"".to_string() | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat unclear mapping of booleans to strings is motivated by the fact that in Python, only empty strings are falsey (e.g. bool("False") == True
); not sure if this serves as motivation to break up getSqlValue
into several functions for the various different literal types.
Codecov Report
@@ Coverage Diff @@
## main #796 +/- ##
==========================================
+ Coverage 74.88% 77.18% +2.29%
==========================================
Files 71 71
Lines 3588 3594 +6
Branches 748 632 -116
==========================================
+ Hits 2687 2774 +87
+ Misses 771 687 -84
- Partials 130 133 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
gpuCI failures here are unrelated - working on a fix in #802 |
|
||
#[pyclass(name = "SqlArg", module = "datafusion")] | ||
#[derive(Debug, Clone, PartialEq, Eq)] | ||
pub struct PySqlArg { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it should be an enum instead of a struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. It might be worth considering changing PySqlArg
from a struct to an enum in the future, as this would simplify some of the pattern matching.
rerun tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes generally lgtm! We can followup in another pr to simplify some structs as suggested by Andy and potentially consolidate of the pysqlArg methods
A continuation of @ChrisJar's work in #752; implements the basic plumbing for
CREATE EXPERIMENT
, and adds aPySqlArg
struct to parser.rs that should allow us to parse nested kwargs in statements that useWITH
.