alamb commented on code in PR #13517:
URL: https://github.com/apache/datafusion/pull/13517#discussion_r1855195122
##########
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:
Ah, I see -- that was non obvious.
Given that what do you think then about making all the fields not `pub`
instead? I think that would be less confusing / more obvious.
Also I do think it is worth considering if we could make a smaller / less
breaking change here -- I understand the desire to ensure only semantically
correct `CreateExternalTable` structures can be created, but in my opinion the
extra cognative overhead of the `*Field` structures makes it significantly
harder to work with this code.
What if we
1. Kept the CreateExternalTableBuilder (to generate errors during
construction)
2. Added another check somewhere when it was processed -- for example in
https://github.com/apache/datafusion/blob/eaf51bab8c97341f10f6a46f2ced366a61089d7d/datafusion/core/src/datasource/listing_table_factory.rs#L54
that there were no duplicate columns in the schema
--
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]
