-
Notifications
You must be signed in to change notification settings - Fork 110
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
feat: sql type checking and logical plan #1708
Conversation
02917ea
to
867084f
Compare
let hex = if hex.starts_with(b"0x") { | ||
&hex[2..] | ||
} else if hex.starts_with(b"X'") { | ||
&hex[2..hex.len()] |
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.
The test compile_eq_identity_address
also check for x
let b = type_ast(*b, tx)?; | ||
assert_eq_types(a.ty(), b.ty())?; | ||
Ok(RelExpr::Union(Box::new(a), Box::new(b))) | ||
} |
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.
Remember to add a comment or check here about checking the schemas are compatible with the union/minus projection
// Equality supports numerics, strings, and bytes | ||
(BinOp::Eq | BinOp::Ne, Type::Alg(t)) | ||
if t.is_bool() | ||
|| t.is_integer() |
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.
I think we should add a trait similar to eq, ord
so we instead do if t.is_comparable
] { | ||
let result = parse_and_type_sub(sql, &tx).inspect_err(|_| { | ||
// println!("sql: {}\n\n\terr: {}\n", sql, err); | ||
}); |
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.
I think this way of testing could blindly let pass an unrelated error and mask something is broken.
If the idea is to make checking for the error easier without heavy matching, how about adding a simple enum Error{TypeCheck, FieldNotFound}
that we return for the error?
field: &str, | ||
expected: Option<&Type>, | ||
) -> TypingResult<(usize, usize, &AlgebraicType)> { | ||
self.find(table) |
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.
I think is better to make a struct that describes what are the usize, usize
here...
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.
I approve after agree with @joshua-spacetime that some changes are for the next step
867084f
to
d4779d2
Compare
Description of Changes
Defines the logical query plan and implements lowering from ast to logical plan which includes variable binding and type checking. DML is still left to do.
API and ABI breaking changes
None
Expected complexity level and risk
3