-
Notifications
You must be signed in to change notification settings - Fork 633
Redshift: UNLOAD #2013
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
Redshift: UNLOAD #2013
Conversation
src/ast/mod.rs
Outdated
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] | ||
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ||
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] | ||
pub struct PartitionBy { |
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.
pub struct PartitionBy { | |
pub struct UnloadPartitionBy { |
Thinking since partitionby has meaning in other contexts?
/// NULL \[ AS \] 'null_string' | ||
Null(String), | ||
/// PARALLEL | ||
Parallel(Option<bool>), |
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.
can we add a description here to mention the behavior of on/off?
src/parser/mod.rs
Outdated
Some(Keyword::GB) => Some(FileSizeUnit::GB), | ||
_ => None, | ||
}; | ||
CopyLegacyOption::RowGroupSize(FileSize { size, unit }) |
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.
Can we pull this out to a function like parse_file_size_unit()
so that it can be reused?
} else { | ||
None | ||
}; | ||
let with = self.parse_options(Keyword::WITH)?; |
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.
Can we add a test case that uses the WITH
keyword in the syntax?
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.
There's already one test case at the start of the unit test. This statement has two flavours, one for Athena (using WITH) and one for Redshift (not using WITH).
3b2bb45
to
dcecedf
Compare
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! Thanks @yoavcloud!
cc @alamb
Added support for the Redshift variant of UNLOAD