Skip to content

ARROW-8058: [Dataset] Relax DatasetFactory discovery validation #6687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
fsaintjacques wants to merge 4 commits into from
Closed

ARROW-8058: [Dataset] Relax DatasetFactory discovery validation #6687

fsaintjacques wants to merge 4 commits into from

Conversation

@fsaintjacques
Copy link
Contributor

@fsaintjacques fsaintjacques commented Mar 23, 2020

This PR aims to improve the latency of the discovery process. Notably, it selects "fast" defaults over "safe" defaults.

  • Add InspectOptions which limits the number of fragments inspected to infer the schema, it defaults to one fragment.
  • Add FinishOptions which toggles if validation of the optional schema and also controls the number of fragments it validates with. It defaults to disabling validation.

This gives a noticeable speedup when the fragments have a uniform schema.

@fsaintjacques fsaintjacques force-pushed the ARROW-8058-optional-discovery-validation branch from 15be905 to 3e70fb7 Compare March 23, 2020 15:15
@github-actions
Copy link

github-actions bot commented Mar 23, 2020

https://issues.apache.org/jira/browse/ARROW-8058

@fsaintjacques fsaintjacques force-pushed the ARROW-8058-optional-discovery-validation branch from 3e70fb7 to 9a8dcea Compare March 23, 2020 16:01
bkietz
bkietz requested changes Mar 23, 2020
View reviewed changes
Copy link
Member

@bkietz bkietz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General approach looks good, a few comments:

jorisvandenbossche
jorisvandenbossche reviewed Mar 23, 2020
View reviewed changes
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looked at the doc comments, the interaction between the different options is not yet fully clear to me

@fsaintjacques fsaintjacques force-pushed the ARROW-8058-optional-discovery-validation branch from ca2a6cb to 4a621de Compare March 25, 2020 13:09
@fsaintjacques fsaintjacques force-pushed the ARROW-8058-optional-discovery-validation branch from 4a621de to 73310eb Compare March 25, 2020 14:07
@fsaintjacques
Copy link
Contributor Author

fsaintjacques commented Mar 25, 2020

I don't think the c/glib failure is related.

bkietz
bkietz approved these changes Mar 25, 2020
View reviewed changes
Copy link
Member

@bkietz bkietz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, merging

(the CI failure is https://issues.apache.org/jira/browse/ARROW-8215 )

@bkietz bkietz closed this in 17b9980 Mar 25, 2020
jorisvandenbossche
jorisvandenbossche reviewed Mar 25, 2020
View reviewed changes
python/pyarrow/tests/test_dataset.py
assert options.partition_base_dir == 'subdir'
assert options.ignore_prefixes == ['.', '_']
assert options.exclude_invalid_files is True
assert options.exclude_invalid_files is False
Copy link
Member

@jorisvandenbossche jorisvandenbossche Mar 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the default changed here?

Copy link
Contributor Author

@fsaintjacques fsaintjacques Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, validates require reading files.

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Mar 25, 2020

This still needs more detailed Python (and R) bindings, right?

@nealrichardson
Copy link
Member

nealrichardson commented Mar 25, 2020

That was my understanding from when we discussed Monday as well.

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Mar 25, 2020

Opened an issue for that https://issues.apache.org/jira/browse/ARROW-8221 (at least for the Python side, R probably needs a similar one?)

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Mar 25, 2020

Still some questions about this:

  • What happens if you put fragments=0? Then the dataset has no known schema? (or is that only possible when specifying a schema manually?)
  • The number of fragments (fragments option) is used to determine how many fragments to use for both when inferring the schema as when validating the schema, right? (but, this number is only used for validating when validate_fragments is set to True)

jorisvandenbossche
jorisvandenbossche reviewed Mar 25, 2020
View reviewed changes
cpp/src/arrow/dataset/discovery.h
/// Indicate if the given Schema (when specified), should be validated against
/// the fragments' schemas. `inspect_options` will control how many fragments
/// are checked.
bool validate_fragments = false;
Copy link
Member

@jorisvandenbossche jorisvandenbossche Mar 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would validate_schema be a better name for this?

@fsaintjacques fsaintjacques deleted the ARROW-8058-optional-discovery-validation branch March 26, 2020 12:07
@fsaintjacques
Copy link
Contributor Author

fsaintjacques commented Mar 26, 2020

Still some questions about this:

  • What happens if you put fragments=0? Then the dataset has no known schema? (or is that only possible when specifying a schema manually?)

You get no schema, or only the one in the partitioning if there's one.

  • The number of fragments (fragments option) is used to determine how many fragments to use for both when inferring the schema as when validating the schema, right? (but, this number is only used for validating when validate_fragments is set to True)

For FinalizeOptions.inspect_options yes. When the schema is not specified, there's no need to validate since the Inspect does some validation while trying to unify.

This was referenced Jun 8, 2020
Closed
Closed
@AlenkaF AlenkaF mentioned this pull request Apr 29, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche left review comments

@bkietz bkietz bkietz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fsaintjacques @jorisvandenbossche @nealrichardson @bkietz