You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "sanjibansg (via GitHub)" <gi...@apache.org> on 2023/05/16 04:20:03 UTC
[GitHub] [arrow] sanjibansg opened a new pull request, #14444: ARROW-17784: [C++] Opening a dataset where partitioning variable is already in the dataset should error differently
sanjibansg opened a new pull request, #14444:
URL: https://github.com/apache/arrow/pull/14444
This PR adds the feature of raising invalid status if the fragment schema and partitioning schema have any common fields, i.e. provided partitioning schema for reading is not the one used for writing.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- closed pull request #14444: ARROW-17784: [C++] Opening a dataset where partitioning variable is already in the dataset should error differently
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- closed pull request #14444: ARROW-17784: [C++] Opening a dataset where partitioning variable is already in the dataset should error differently
URL: https://github.com/apache/arrow/pull/14444
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on a diff in pull request #14444: ARROW-17784: [C++] Opening a dataset where partitioning variable is already in the dataset should error differently
Posted by GitBox <gi...@apache.org>.
thisisnic commented on code in PR #14444:
URL: https://github.com/apache/arrow/pull/14444#discussion_r1016569924
##########
cpp/src/arrow/dataset/discovery.cc:
##########
@@ -236,25 +236,35 @@ Result<std::vector<std::shared_ptr<Schema>>> FileSystemDatasetFactory::InspectSc
InspectOptions options) {
std::vector<std::shared_ptr<Schema>> schemas;
+ ARROW_ASSIGN_OR_RAISE(auto partition_schema,
+ options_.partitioning.GetOrInferSchema(
+ StripPrefixAndFilename(files_, options_.partition_base_dir)));
+
const bool has_fragments_limit = options.fragments >= 0;
int fragments = options.fragments;
for (const auto& info : files_) {
if (has_fragments_limit && fragments-- == 0) break;
auto result = format_->Inspect({info, fs_});
+
if (ARROW_PREDICT_FALSE(!result.ok())) {
return result.status().WithMessage(
"Error creating dataset. Could not read schema from '", info.path(),
"': ", result.status().message(), ". Is this a '", format_->type_name(),
"' file?");
}
+
+ if (partition_schema->num_fields()) {
+ auto field_check =
+ result->get()->CanReferenceFieldsByNames(partition_schema->field_names());
+ if (ARROW_PREDICT_FALSE(field_check.ok())) {
+ return Status::Invalid(
+ "Error creating dataset. Partitioning field(s) present in fragment.");
+ }
+ }
Review Comment:
My preference would be either silently ignoring it or making it configurable (with one of the configurable options being to silently ignore it).
Users might inherit poorly-designed datasets that they have no control over how they've been written, and I'd hate for it to be impossible for them to be able to work with them in Arrow and just get an error.
I'm not sure how/if having a dataset with 2 columns with the same name would work in R as I think it'd trigger an error somewhere (if it didn't, I think we'd maybe want to make it do so?), but if it'd be useful to have that as a feature, we can just catch it in R and add our own custom error, so I don't see a problem in that.
Silently ignoring would be fine too, as we can always add our own error in R if we want to warn users that there's duplication, and it at least allows execution.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ARROW-17784: [C++] Opening a dataset where partitioning variable is already in the dataset should error differently [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #14444: ARROW-17784: [C++] Opening a dataset where partitioning variable is already in the dataset should error differently
URL: https://github.com/apache/arrow/pull/14444
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #14444: ARROW-17784: [C++] Opening a dataset where partitioning variable is already in the dataset should error differently
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #14444:
URL: https://github.com/apache/arrow/pull/14444#discussion_r1016037559
##########
cpp/src/arrow/dataset/discovery.cc:
##########
@@ -236,25 +236,35 @@ Result<std::vector<std::shared_ptr<Schema>>> FileSystemDatasetFactory::InspectSc
InspectOptions options) {
std::vector<std::shared_ptr<Schema>> schemas;
+ ARROW_ASSIGN_OR_RAISE(auto partition_schema,
+ options_.partitioning.GetOrInferSchema(
+ StripPrefixAndFilename(files_, options_.partition_base_dir)));
+
const bool has_fragments_limit = options.fragments >= 0;
int fragments = options.fragments;
for (const auto& info : files_) {
if (has_fragments_limit && fragments-- == 0) break;
auto result = format_->Inspect({info, fs_});
+
if (ARROW_PREDICT_FALSE(!result.ok())) {
return result.status().WithMessage(
"Error creating dataset. Could not read schema from '", info.path(),
"': ", result.status().message(), ". Is this a '", format_->type_name(),
"' file?");
}
+
+ if (partition_schema->num_fields()) {
+ auto field_check =
+ result->get()->CanReferenceFieldsByNames(partition_schema->field_names());
+ if (ARROW_PREDICT_FALSE(field_check.ok())) {
+ return Status::Invalid(
+ "Error creating dataset. Partitioning field(s) present in fragment.");
+ }
+ }
Review Comment:
This will catch a problem if the error is noticed during inspection but sometimes we don't use all fragments to inspect a dataset and other times we might not do dataset inspection at all (e.g. if the user provides the dataset schema and the partitioning schema). I wonder if we might want to check somewhere at scan time instead of during discovery.
Also, in some cases, maybe this is not always a bad thing. I seem to recall users would sometimes store the schema information in a column in the file in addition to the filename. Maybe a better behavior would be to silently ignore the column in the file if there is partitioning information that specifies a given column. Or at least to make it configurable (ignore vs error vs two columns with the same name). @thisisnic any preference?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ARROW-17784: [C++] Opening a dataset where partitioning variable is already in the dataset should error differently [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14444:
URL: https://github.com/apache/arrow/pull/14444#issuecomment-3547079365
Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14444: ARROW-17784: [C++] Opening a dataset where partitioning variable is already in the dataset should error differently
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14444:
URL: https://github.com/apache/arrow/pull/14444#issuecomment-1281932546
https://issues.apache.org/jira/browse/ARROW-17784
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- commented on pull request #14444: ARROW-17784: [C++] Opening a dataset where partitioning variable is already in the dataset should error differently
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- commented on PR #14444:
URL: https://github.com/apache/arrow/pull/14444#issuecomment-1490646639
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org