You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kumarUjjawal (via GitHub)" <gi...@apache.org> on 2026/06/01 11:18:38 UTC
[PR] feat: Support multiple external table locations [datafusion]
kumarUjjawal opened a new pull request, #22695:
URL: https://github.com/apache/datafusion/pull/22695
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Part of #16303.
## Rationale for this change
`CREATE EXTERNAL TABLE` can reference only one location today. This adds support for listing multiple explicit locations and reading them as one table.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
- Adds `LOCATION ('a.parquet', 'b.parquet')` syntax.
- Keeps `LOCATION 'a,b.parquet'` as a single path, so literal commas still work.
- Carries multiple locations through the logical plan and proto.
- Updates listing table creation to scan all listed locations.
- Requires all listed locations to use the same object store and matching fields.
- Keeps stream tables limited to exactly one location.
- Updates docs and upgrade notes.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
Yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
Yes. `CREATE EXTERNAL TABLE` now accepts a parenthesized list of locations.
There is also a public API change: `CreateExternalTable.location` is replaced by `CreateExternalTable.locations`.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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
Re: [PR] feat: Support multiple external table locations [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal merged PR #22695:
URL: https://github.com/apache/datafusion/pull/22695
--
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
Re: [PR] feat: Support multiple external table locations [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #22695:
URL: https://github.com/apache/datafusion/pull/22695#issuecomment-4999373822
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/22695?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `86.74242%` with `35 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 80.65%. Comparing base ([`7ca6e54`](https://app.codecov.io/gh/apache/datafusion/commit/7ca6e543ec30ab0413b75171747a6d39ac0b0537?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8d22b92`](https://app.codecov.io/gh/apache/datafusion/commit/8d22b9206a931eb0b8d7110e193b87654d1fea35?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/22695?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/proto-models/src/generated/pbjson.rs](https://app.codecov.io/gh/apache/datafusion/pull/22695?src=pr&el=tree&filepath=datafusion%2Fproto-models%2Fsrc%2Fgenerated%2Fpbjson.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9wcm90by1tb2RlbHMvc3JjL2dlbmVyYXRlZC9wYmpzb24ucnM=) | 0.00% | [13 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/22695?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/sql/src/parser.rs](https://app.codecov.io/gh/apache/datafusion/pull/22695?src=pr&el=tree&filepath=datafusion%2Fsql%2Fsrc%2Fparser.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zcWwvc3JjL3BhcnNlci5ycw==) | 88.46% | [4 Missing and 8 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/22695?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...usion/core/src/datasource/listing\_table\_factory.rs](https://app.codecov.io/gh/apache/datafusion/pull/22695?src=pr&el=tree&filepath=datafusion%2Fcore%2Fsrc%2Fdatasource%2Flisting_table_factory.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb3JlL3NyYy9kYXRhc291cmNlL2xpc3RpbmdfdGFibGVfZmFjdG9yeS5ycw==) | 96.63% | [3 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/22695?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/proto/src/logical\_plan/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/22695?src=pr&el=tree&filepath=datafusion%2Fproto%2Fsrc%2Flogical_plan%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9wcm90by9zcmMvbG9naWNhbF9wbGFuL21vZC5ycw==) | 76.92% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/22695?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/expr/src/logical\_plan/ddl.rs](https://app.codecov.io/gh/apache/datafusion/pull/22695?src=pr&el=tree&filepath=datafusion%2Fexpr%2Fsrc%2Flogical_plan%2Fddl.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy9sb2dpY2FsX3BsYW4vZGRsLnJz) | 77.77% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/22695?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/sql/src/statement.rs](https://app.codecov.io/gh/apache/datafusion/pull/22695?src=pr&el=tree&filepath=datafusion%2Fsql%2Fsrc%2Fstatement.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zcWwvc3JjL3N0YXRlbWVudC5ycw==) | 75.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/22695?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #22695 +/- ##
==========================================
- Coverage 80.66% 80.65% -0.01%
==========================================
Files 1086 1086
Lines 366694 366747 +53
Branches 366694 366747 +53
==========================================
+ Hits 295806 295818 +12
- Misses 53265 53293 +28
- Partials 17623 17636 +13
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/22695?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] feat: Support multiple external table locations [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #22695:
URL: https://github.com/apache/datafusion/pull/22695#issuecomment-4598904704
Thank you @martin-g for great feedback. I have addressed the concerns.
--
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
Re: [PR] feat: Support multiple external table locations [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #22695:
URL: https://github.com/apache/datafusion/pull/22695#discussion_r3600520548
##########
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:
I will add a test
--
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
Re: [PR] feat: Support multiple external table locations [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #22695:
URL: https://github.com/apache/datafusion/pull/22695#discussion_r3600522411
##########
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:
Agree!
--
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
Re: [PR] feat: Support multiple external table locations [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #22695:
URL: https://github.com/apache/datafusion/pull/22695#issuecomment-5018559749
Thank you @Jefffrey
--
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
Re: [PR] feat: Support multiple external table locations [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #22695:
URL: https://github.com/apache/datafusion/pull/22695#issuecomment-4592240387
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion v53.1.0 (current)
Built [ 98.482s] (current)
Parsing datafusion v53.1.0 (current)
Parsed [ 0.038s] (current)
Building datafusion v53.1.0 (baseline)
Built [ 98.981s] (baseline)
Parsing datafusion v53.1.0 (baseline)
Parsed [ 0.039s] (baseline)
Checking datafusion v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.889s] 222 checks: 222 pass, 30 skip
Summary no semver update required
Finished [ 200.491s] datafusion
Building datafusion-catalog v53.1.0 (current)
Built [ 37.382s] (current)
Parsing datafusion-catalog v53.1.0 (current)
Parsed [ 0.027s] (current)
Building datafusion-catalog v53.1.0 (baseline)
Built [ 36.873s] (baseline)
Parsing datafusion-catalog v53.1.0 (baseline)
Parsed [ 0.029s] (baseline)
Checking datafusion-catalog v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.176s] 222 checks: 222 pass, 30 skip
Summary no semver update required
Finished [ 76.043s] datafusion-catalog
Building datafusion-cli v53.1.0 (current)
Built [ 170.023s] (current)
Parsing datafusion-cli v53.1.0 (current)
Parsed [ 0.035s] (current)
Building datafusion-cli v53.1.0 (baseline)
Built [ 168.758s] (baseline)
Parsing datafusion-cli v53.1.0 (baseline)
Parsed [ 0.041s] (baseline)
Checking datafusion-cli v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.144s] 222 checks: 222 pass, 30 skip
Summary no semver update required
Finished [ 343.230s] datafusion-cli
Building datafusion-expr v53.1.0 (current)
Built [ 25.454s] (current)
Parsing datafusion-expr v53.1.0 (current)
Parsed [ 0.078s] (current)
Building datafusion-expr v53.1.0 (baseline)
Built [ 25.382s] (baseline)
Parsing datafusion-expr v53.1.0 (baseline)
Parsed [ 0.079s] (baseline)
Checking datafusion-expr v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 1.697s] 222 checks: 220 pass, 2 fail, 0 warn, 30 skip
--- failure constructible_struct_adds_field: externally-constructible struct adds field ---
Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/constructible_struct_adds_field.ron
Failed in:
field CreateExternalTable.locations in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:219
field CreateExternalTable.locations in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:219
--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---
Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/struct_pub_field_missing.ron
Failed in:
field location of struct CreateExternalTable, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/06dd7115d01d285840987edcd0dd32a5798a198a/datafusion/expr/src/logical_plan/ddl.rs:215
field location of struct CreateExternalTable, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/06dd7115d01d285840987edcd0dd32a5798a198a/datafusion/expr/src/logical_plan/ddl.rs:215
Summary semver requires new major version: 2 major and 0 minor checks failed
Finished [ 53.750s] datafusion-expr
Building datafusion-ffi v53.1.0 (current)
Built [ 57.910s] (current)
Parsing datafusion-ffi v53.1.0 (current)
Parsed [ 0.061s] (current)
Building datafusion-ffi v53.1.0 (baseline)
Built [ 57.839s] (baseline)
Parsing datafusion-ffi v53.1.0 (baseline)
Parsed [ 0.064s] (baseline)
Checking datafusion-ffi v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.336s] 222 checks: 222 pass, 30 skip
Summary no semver update required
Finished [ 117.963s] datafusion-ffi
Building datafusion-proto v53.1.0 (current)
Built [ 56.273s] (current)
Parsing datafusion-proto v53.1.0 (current)
Parsed [ 0.020s] (current)
Building datafusion-proto v53.1.0 (baseline)
Built [ 56.793s] (baseline)
Parsing datafusion-proto v53.1.0 (baseline)
Parsed [ 0.020s] (baseline)
Checking datafusion-proto v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.357s] 222 checks: 222 pass, 30 skip
Summary no semver update required
Finished [ 115.785s] datafusion-proto
Building datafusion-proto-models v53.1.0 (current)
Built [ 23.632s] (current)
Parsing datafusion-proto-models v53.1.0 (current)
Parsed [ 0.135s] (current)
Building datafusion-proto-models v53.1.0 (baseline)
Built [ 23.313s] (baseline)
Parsing datafusion-proto-models v53.1.0 (baseline)
Parsed [ 0.133s] (baseline)
Checking datafusion-proto-models v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 2.272s] 222 checks: 220 pass, 2 fail, 0 warn, 30 skip
--- failure constructible_struct_adds_field: externally-constructible struct adds field ---
Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/constructible_struct_adds_field.ron
Failed in:
field CreateExternalTableNode.locations in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:247
field CreateExternalTableNode.locations in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:247
--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---
Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/struct_pub_field_missing.ron
Failed in:
field location of struct CreateExternalTableNode, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/06dd7115d01d285840987edcd0dd32a5798a198a/datafusion/proto-models/src/generated/prost.rs:247
field location of struct CreateExternalTableNode, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/06dd7115d01d285840987edcd0dd32a5798a198a/datafusion/proto-models/src/generated/prost.rs:247
Summary semver requires new major version: 2 major and 0 minor checks failed
Finished [ 50.723s] datafusion-proto-models
Building datafusion-sql v53.1.0 (current)
Built [ 39.938s] (current)
Parsing datafusion-sql v53.1.0 (current)
Parsed [ 0.034s] (current)
Building datafusion-sql v53.1.0 (baseline)
Built [ 40.236s] (baseline)
Parsing datafusion-sql v53.1.0 (baseline)
Parsed [ 0.035s] (baseline)
Checking datafusion-sql v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.320s] 222 checks: 221 pass, 1 fail, 0 warn, 30 skip
--- failure constructible_struct_adds_field: externally-constructible struct adds field ---
Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/constructible_struct_adds_field.ron
Failed in:
field CreateExternalTable.locations in /home/runner/work/datafusion/datafusion/datafusion/sql/src/parser.rs:255
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 82.361s] datafusion-sql
Building datafusion-sqllogictest v53.1.0 (current)
Built [ 169.182s] (current)
Parsing datafusion-sqllogictest v53.1.0 (current)
Parsed [ 0.024s] (current)
Building datafusion-sqllogictest v53.1.0 (baseline)
Built [ 169.428s] (baseline)
Parsing datafusion-sqllogictest v53.1.0 (baseline)
Parsed [ 0.025s] (baseline)
Checking datafusion-sqllogictest v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.108s] 222 checks: 222 pass, 30 skip
Summary no semver update required
Finished [ 343.592s] datafusion-sqllogictest
```
</details>
--
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
Re: [PR] feat: Support multiple external table locations [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #22695:
URL: https://github.com/apache/datafusion/pull/22695#discussion_r3600525679
##########
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:
This would be a good followup. Thanks!
--
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
Re: [PR] feat: Support multiple external table locations [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #22695:
URL: https://github.com/apache/datafusion/pull/22695#issuecomment-4768023512
@Jefffrey do you have time to review this pr?
--
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
Re: [PR] feat: Support multiple external table locations [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
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
Re: [PR] feat: Support multiple external table locations [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #22695:
URL: https://github.com/apache/datafusion/pull/22695#discussion_r3600514075
##########
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:
Locations are now rendered as valid SQL string literals.
--
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
Re: [PR] feat: Support multiple external table locations [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #22695:
URL: https://github.com/apache/datafusion/pull/22695#discussion_r3334456186
##########
datafusion/core/src/datasource/listing_table_factory.rs:
##########
@@ -222,13 +297,18 @@ fn get_extension(path: &str) -> String {
}
}
+fn schemas_have_same_fields(left: &SchemaRef, right: &SchemaRef) -> bool {
+ left.fields() == right.fields()
Review Comment:
https://github.com/apache/datafusion/pull/22695/changes#diff-8ce463e0d56c67a237fce2942f0759984d56764b13b62156472132ab84c78192R226-R227 says `Schema metadata may differ between files ...` but here the equality check compares the fields' metadata too - https://docs.rs/arrow-schema/58.3.0/src/arrow_schema/field.rs.html#113
Is this intentional ?
##########
datafusion/proto-models/src/generated/prost.rs:
##########
@@ -243,8 +243,8 @@ pub struct EmptyRelationNode {
pub struct CreateExternalTableNode {
#[prost(message, optional, tag = "9")]
pub name: ::core::option::Option<TableReference>,
- #[prost(string, tag = "2")]
- pub location: ::prost::alloc::string::String,
Review Comment:
What is the lifetime of a serialized CreateExternalTableNode ?
If there is a use case where a CreateExternalTableNode serialized with DF v.X and deserialized with v. X+1 then it would be better to keep `location` around until v. X+2 (assuming users don't upgrade from X to X+2).
I.e. deserialize should do:
1) if `locations` is not empty then use it
2) otherwise if `location` is not an empty string then insert it to `locations`
Serialize should: 1) write non-empty `locations` and `location=""`
##########
datafusion/core/src/test_util/mod.rs:
##########
@@ -188,7 +188,7 @@ impl TableProviderFactory for TestTableFactory {
cmd: &CreateExternalTable,
) -> Result<Arc<dyn TableProvider>> {
Ok(Arc::new(TestTableProvider {
- url: cmd.location.to_string(),
+ url: cmd.locations.first().cloned().unwrap_or_default(),
Review Comment:
This could silently use `""` if no locations are provided. Better return an error.
##########
datafusion/core/src/datasource/listing_table_factory.rs:
##########
@@ -142,36 +182,71 @@ 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<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
+ // metadata may differ between files without changing the
+ // fields read by the table.
+ match &inferred_schema {
+ None => inferred_schema = Some(schema),
+ Some(existing)
+ if !schemas_have_same_fields(existing, &schema) =>
+ {
+ return plan_err!(
+ "All locations of a CREATE EXTERNAL TABLE must have the \
+ same schema, but the provided locations have differing schemas"
Review Comment:
This error message does not help the user to find the problematic schema/location.
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -795,10 +725,15 @@ impl AsLogicalPlan for LogicalPlanNode {
create_extern_table.name.as_ref(),
"CreateExternalTable",
)?,
- create_extern_table.location.clone(),
+ create_extern_table
Review Comment:
If backward compatibility is needed, i.e. if a CreateExternalTableNode is serialized with v. X and it has to be read with v. X+1 then you need to keep `location` around for at least one major version.
If such compatibility is not needed then you can just use `""` here. It is overwritten by `.with_locations(...)` below
##########
datafusion/sql/src/parser.rs:
##########
@@ -289,7 +257,18 @@ impl fmt::Display for CreateExternalTable {
}
write!(f, ") ")?;
}
- write!(f, "LOCATION {}", self.location)
+ if self.locations.len() > 1 {
+ write!(f, "LOCATION (")?;
+ for (idx, location) in self.locations.iter().enumerate() {
+ if idx > 0 {
+ write!(f, ", ")?;
+ }
+ write!(f, "{location}")?;
+ }
+ write!(f, ")")
+ } else {
+ write!(f, "LOCATION {}", self.location)
Review Comment:
I see that this is how it worked before but shouldn't the locations be quoted ?
--
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