You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Jefffrey (via GitHub)" <gi...@apache.org> on 2026/07/16 04:33:10 UTC
Re: [PR] feat: Support multiple external table locations [datafusion]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org