Jefffrey commented on code in PR #22695:
URL: https://github.com/apache/datafusion/pull/22695#discussion_r3592620143
##########
datafusion/sqllogictest/test_files/information_schema.slt:
##########
@@ -777,7 +777,7 @@ OPTIONS ('format.has_header' 'true');
query TTTT
SHOW CREATE TABLE abc;
----
-datafusion public abc CREATE EXTERNAL TABLE abc STORED AS CSV LOCATION
../../testing/data/csv/aggregate_test_100.csv
+datafusion public abc CREATE EXTERNAL TABLE abc STORED AS CSV LOCATION
'../../testing/data/csv/aggregate_test_100.csv'
Review Comment:
is this quoting a side effect of this change or just formatting/linting?
##########
datafusion/sqllogictest/test_files/create_external_table.slt:
##########
@@ -303,3 +303,35 @@ statement error DataFusion error: SQL error:
ParserError\("'IF NOT EXISTS' canno
CREATE OR REPLACE EXTERNAL TABLE IF NOT EXISTS t_conflict(c1 int)
STORED AS CSV
LOCATION 'foo.csv';
+
+# Multiple listed locations are read together as a single table.
+# Each partition-N.csv has 11 rows, so listing exactly two of them (rather than
+# the whole directory) yields 22 rows.
+statement ok
+CREATE EXTERNAL TABLE multi_loc (c1 int, c2 bigint, c3 boolean)
+STORED AS CSV
+LOCATION ('../core/tests/data/partitioned_csv/partition-0.csv',
'../core/tests/data/partitioned_csv/partition-1.csv')
Review Comment:
does anything funky happen if you specify the same location twice?
##########
docs/source/user-guide/sql/ddl.md:
##########
@@ -82,6 +82,22 @@ For a comprehensive list of format-specific options that can
be specified in the
a path to a file or directory of partitioned files locally or on an
object store.
+Multiple locations can be supplied as a parenthesized list of string literals,
+in which case the files are read together as one table:
+
+```sql
+CREATE EXTERNAL TABLE hits
+STORED AS PARQUET
+LOCATION (
+
's3://clickhouse-public-datasets/hits_compatible/athena_partitioned/hits_1.parquet',
+
's3://clickhouse-public-datasets/hits_compatible/athena_partitioned/hits_2.parquet'
+);
+```
+
+All listed locations must reside on the same object store and resolve to data
+with the same schema. Paths that themselves contain a literal comma can still
Review Comment:
i wonder if we should omit the detail on paths containing a comma; it seems
to be extra detail that isnt really needed because we can see the paths are
already enclosed as literal strings anyway 🤔
##########
datafusion/sql/src/parser.rs:
##########
@@ -249,8 +249,10 @@ pub struct CreateExternalTable {
pub columns: Vec<ColumnDef>,
/// File type (Parquet, NDJSON, CSV, etc)
pub file_type: String,
- /// Path to file
+ /// First path to file, retained for backwards compatibility.
Review Comment:
i do wonder if we should take this opportunity to do a clean cut to
`locations` since its already an API change or too many downstream users rely
on `location` and prevents us from doing so 🤔
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -1820,7 +1832,8 @@ impl AsLogicalPlan for LogicalPlanNode {
name: Some(protobuf::TableReference::from_proto(
name.clone(),
)),
- location: location.clone(),
+ location:
locations.first().cloned().unwrap_or_default(),
Review Comment:
is it better to just leave location empty? otherwise it could have subtle
behaviour like dropping other locations if multiple were specified 🤔
##########
datafusion/sql/src/statement.rs:
##########
@@ -1805,7 +1805,8 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
name,
columns,
file_type,
- location,
+ location: _,
Review Comment:
should we add a comment here explaining why we ignore `location`?
##########
datafusion/sql/src/statement.rs:
##########
@@ -1854,9 +1855,15 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
let name = self.object_name_to_table_reference(name)?;
let constraints =
self.new_constraint_from_table_constraints(&all_constraints,
&df_schema)?;
+
+ let Some(location) = locations.first().cloned() else {
+ return plan_err!("CREATE EXTERNAL TABLE requires at least one
location");
+ };
+
Ok(LogicalPlan::Ddl(DdlStatement::CreateExternalTable(
Box::new(
PlanCreateExternalTable::builder(name, location, file_type,
df_schema)
Review Comment:
similarly here do we need to explain why we have to provide the first
location via builder but then override (i assume?) using `with_locations()`
after?
##########
datafusion/core/src/datasource/listing_table_factory.rs:
##########
@@ -141,36 +181,75 @@ impl TableProviderFactory for ListingTableFactory {
options = options.with_table_partition_cols(table_partition_cols);
- options
- .validate_partitions(session_state, &table_path)
- .await?;
+ // Validate partitions against every location before any glob
rewriting.
+ for table_path in &table_paths {
+ options
+ .validate_partitions(session_state, table_path)
+ .await?;
+ }
- let resolved_schema = match provided_schema {
+ let (resolved_table_paths, resolved_schema) = match provided_schema {
// We will need to check the table columns against the schema
// this is done so that we can do an ORDER BY for external table
creation
// specifically for parquet file format.
// See: https://github.com/apache/datafusion/issues/7317
None => {
- // if the folder then rewrite a file path as 'path/*.parquet'
- // to only read the files the reader can understand
- if table_path.is_folder() && table_path.get_glob().is_none() {
- // Since there are no files yet to infer an actual
extension,
- // derive the pattern based on compression type.
- // So for gzipped CSV the pattern is `*.csv.gz`
- let glob = match options.format.compression_type() {
- Some(compression) => {
- match
options.format.get_ext_with_compression(&compression) {
- // Use glob based on `FileFormat` extension
- Ok(ext) => format!("*.{ext}"),
- // Fallback to `file_type`, if not supported
by `FileFormat`
- Err(_) => format!("*.{}",
cmd.file_type.to_lowercase()),
+ let mut resolved_paths = Vec::with_capacity(table_paths.len());
+ let mut inferred_schema: Option<(String, SchemaRef)> = None;
+ for mut table_path in table_paths {
+ // if the folder then rewrite a file path as
'path/*.parquet'
+ // to only read the files the reader can understand
+ if table_path.is_folder() &&
table_path.get_glob().is_none() {
+ // Since there are no files yet to infer an actual
extension,
+ // derive the pattern based on compression type.
+ // So for gzipped CSV the pattern is `*.csv.gz`
+ let glob = match options.format.compression_type() {
+ Some(compression) => {
+ match options
+ .format
+ .get_ext_with_compression(&compression)
+ {
+ // Use glob based on `FileFormat` extension
+ Ok(ext) => format!("*.{ext}"),
+ // Fallback to `file_type`, if not
supported by `FileFormat`
+ Err(_) => {
+ format!("*.{}",
cmd.file_type.to_lowercase())
+ }
+ }
}
+ None => format!("*.{}",
cmd.file_type.to_lowercase()),
+ };
+ table_path = table_path.with_glob(glob.as_ref())?;
+ }
+ let schema = options.infer_schema(session_state,
&table_path).await?;
+ // All locations must resolve to the same fields. Schema
+ // and field metadata may differ between files without
+ // changing the fields read by the table.
+ let location = table_path.to_string();
+ match &inferred_schema {
+ None => inferred_schema = Some((location, schema)),
+ Some((existing_location, existing))
+ if !schemas_have_same_fields(existing, &schema) =>
Review Comment:
i suppose a followup issue could be allowing users to customize this, for
example filling nulls for missing columns; but this is a good initial rule
##########
datafusion/core/src/datasource/listing_table_factory.rs:
##########
@@ -221,13 +300,27 @@ fn get_extension(path: &str) -> String {
}
}
+fn schemas_have_same_fields(left: &SchemaRef, right: &SchemaRef) -> bool {
Review Comment:
would a followup be exploring if we should allow union by name instead of
only union by order
--
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]
