alamb commented on code in PR #13517:
URL: https://github.com/apache/datafusion/pull/13517#discussion_r1855185944
##########
datafusion/expr/src/logical_plan/ddl.rs:
##########
@@ -303,12 +531,154 @@ pub struct CreateMemoryTable {
pub or_replace: bool,
/// Default values for columns
pub column_defaults: Vec<(String, Expr)>,
- /// Wheter the table is `TableType::Temporary`
+ /// Whether the table is `TableType::Temporary`
pub temporary: bool,
}
+impl CreateMemoryTable {
+ pub fn new(fields: CreateMemoryTableFields) -> Result<Self> {
Review Comment:
It would be nice to call this `try_new()` to signal it can return an error
##########
datafusion/core/src/catalog_common/listing_schema.rs:
##########
@@ -131,21 +129,12 @@ impl ListingSchemaProvider {
.factory
.create(
state,
- &CreateExternalTable {
- schema: Arc::new(DFSchema::empty()),
- name,
- location: table_url,
- file_type: self.format.clone(),
- table_partition_cols: vec![],
- if_not_exists: false,
- temporary: false,
- definition: None,
- order_exprs: vec![],
- unbounded: false,
- options: Default::default(),
- constraints: Constraints::empty(),
- column_defaults: Default::default(),
- },
+ &CreateExternalTable::builder()
Review Comment:
😍
##########
datafusion/expr/src/logical_plan/ddl.rs:
##########
@@ -192,11 +193,12 @@ impl DdlStatement {
/// Creates an external table.
#[derive(Debug, Clone, PartialEq, Eq)]
+#[non_exhaustive]
Review Comment:
Why make it non exhaustive? That might make it harder to write `match`
statements where all the fields are matched (and the compiler tells you when
new ones are added)
##########
datafusion/expr/src/logical_plan/ddl.rs:
##########
@@ -288,8 +290,234 @@ impl PartialOrd for CreateExternalTable {
}
}
+impl CreateExternalTable {
+ pub fn new(fields: CreateExternalTableFields) -> Result<Self> {
Review Comment:
Given all the fields in `CreateExternalTable` are `pub` anyway, I don't
think introducing `CreateExternalTableFields` requires all construction to go
through `new` 🤔
Therefore I would personally suggest removing the
`CreateExternalTableFields` (and equivalent for MemoryTable) and staying with
the struct and the builder.
##########
datafusion/expr/src/logical_plan/ddl.rs:
##########
@@ -288,8 +290,234 @@ impl PartialOrd for CreateExternalTable {
}
}
+impl CreateExternalTable {
+ pub fn new(fields: CreateExternalTableFields) -> Result<Self> {
+ let CreateExternalTableFields {
+ name,
+ schema,
+ location,
+ file_type,
+ table_partition_cols,
+ if_not_exists,
+ temporary,
+ definition,
+ order_exprs,
+ unbounded,
+ options,
+ constraints,
+ column_defaults,
+ } = fields;
+ check_fields_unique(&schema)?;
+ Ok(Self {
+ name,
+ schema,
+ location,
+ file_type,
+ table_partition_cols,
+ if_not_exists,
+ temporary,
+ definition,
+ order_exprs,
+ unbounded,
+ options,
+ constraints,
+ column_defaults,
+ })
+ }
+
+ pub fn into_fields(self) -> CreateExternalTableFields {
+ let Self {
+ name,
+ schema,
+ location,
+ file_type,
+ table_partition_cols,
+ if_not_exists,
+ temporary,
+ definition,
+ order_exprs,
+ unbounded,
+ options,
+ constraints,
+ column_defaults,
+ } = self;
+ CreateExternalTableFields {
+ name,
+ schema,
+ location,
+ file_type,
+ table_partition_cols,
+ if_not_exists,
+ temporary,
+ definition,
+ order_exprs,
+ unbounded,
+ options,
+ constraints,
+ column_defaults,
+ }
+ }
+
+ pub fn builder() -> CreateExternalTableBuilder {
+ CreateExternalTableBuilder::new()
+ }
+}
+
+/// A struct with same fields as [`CreateExternalTable`] struct so that the
DDL can be conveniently
+/// destructed with validation that each field is handled, while still
requiring that all
+/// construction goes through the [`CreateExternalTable::new`] constructor or
the builder.
+pub struct CreateExternalTableFields {
+ /// The table name
+ pub name: TableReference,
+ /// The table schema
+ pub schema: DFSchemaRef,
+ /// The physical location
+ pub location: String,
+ /// The file type of physical file
+ pub file_type: String,
+ /// Partition Columns
+ pub table_partition_cols: Vec<String>,
+ /// Option to not error if table already exists
+ pub if_not_exists: bool,
+ /// Whether the table is a temporary table
+ pub temporary: bool,
+ /// SQL used to create the table, if available
+ pub definition: Option<String>,
+ /// Order expressions supplied by user
+ pub order_exprs: Vec<Vec<Sort>>,
+ /// Whether the table is an infinite streams
+ pub unbounded: bool,
+ /// Table(provider) specific options
+ pub options: HashMap<String, String>,
+ /// The list of constraints in the schema, such as primary key, unique,
etc.
+ pub constraints: Constraints,
+ /// Default values for columns
+ pub column_defaults: HashMap<String, Expr>,
+}
+
+/// A builder or [`CreateExternalTable`]. Use [`CreateExternalTable::builder`]
to obtain a new builder instance.
+pub struct CreateExternalTableBuilder {
+ name: Option<TableReference>,
+ schema: Option<DFSchemaRef>,
+ location: Option<String>,
+ file_type: Option<String>,
+ table_partition_cols: Vec<String>,
+ if_not_exists: bool,
+ temporary: bool,
+ definition: Option<String>,
+ order_exprs: Vec<Vec<Sort>>,
+ unbounded: bool,
+ options: HashMap<String, String>,
+ constraints: Constraints,
+ column_defaults: HashMap<String, Expr>,
+}
+
+impl CreateExternalTableBuilder {
+ fn new() -> Self {
+ Self {
+ name: None,
+ schema: None,
+ location: None,
+ file_type: None,
+ table_partition_cols: vec![],
+ if_not_exists: false,
+ temporary: false,
+ definition: None,
+ order_exprs: vec![],
+ unbounded: false,
+ options: HashMap::new(),
+ constraints: Constraints::empty(),
+ column_defaults: HashMap::new(),
+ }
+ }
+
+ pub fn name(mut self, name: TableReference) -> Self {
+ self.name = Some(name);
+ self
+ }
+
+ pub fn schema(mut self, schema: DFSchemaRef) -> Self {
+ self.schema = Some(schema);
+ self
+ }
+
+ pub fn location(mut self, location: String) -> Self {
+ self.location = Some(location);
+ self
+ }
+
+ pub fn file_type(mut self, file_type: String) -> Self {
+ self.file_type = Some(file_type);
+ self
+ }
+
+ pub fn table_partition_cols(mut self, table_partition_cols: Vec<String>)
-> Self {
+ self.table_partition_cols = table_partition_cols;
+ self
+ }
+
+ pub fn if_not_exists(mut self, if_not_exists: bool) -> Self {
+ self.if_not_exists = if_not_exists;
+ self
+ }
+
+ pub fn temporary(mut self, temporary: bool) -> Self {
+ self.temporary = temporary;
+ self
+ }
+
+ pub fn definition(mut self, definition: Option<String>) -> Self {
+ self.definition = definition;
+ self
+ }
+
+ pub fn order_exprs(mut self, order_exprs: Vec<Vec<Sort>>) -> Self {
+ self.order_exprs = order_exprs;
+ self
+ }
+
+ pub fn unbounded(mut self, unbounded: bool) -> Self {
+ self.unbounded = unbounded;
+ self
+ }
+
+ pub fn options(mut self, options: HashMap<String, String>) -> Self {
+ self.options = options;
+ self
+ }
+
+ pub fn constraints(mut self, constraints: Constraints) -> Self {
+ self.constraints = constraints;
+ self
+ }
+
+ pub fn column_defaults(mut self, column_defaults: HashMap<String, Expr>)
-> Self {
+ self.column_defaults = column_defaults;
+ self
+ }
+
+ pub fn build(self) -> Result<CreateExternalTable> {
+ CreateExternalTable::new(CreateExternalTableFields {
+ name: self.name.expect("name is required"),
+ schema: self.schema.expect("schema is required"),
+ location: self.location.expect("location is required"),
+ file_type: self.file_type.expect("file_type is required"),
+ table_partition_cols: self.table_partition_cols,
+ if_not_exists: self.if_not_exists,
+ temporary: self.temporary,
+ definition: self.definition,
+ order_exprs: self.order_exprs,
+ unbounded: self.unbounded,
+ options: self.options,
+ constraints: self.constraints,
+ column_defaults: self.column_defaults,
+ })
+ }
+}
+
/// Creates an in memory table.
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
+#[non_exhaustive]
Review Comment:
why add this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
