ARROW-7366: [C++][Dataset] Use PartitionSchemeDiscovery in DataSourceDiscovery

[bkietz]

No description provided.

[github-actions]

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

[fsaintjacques]

Needs a rebase.

[fsaintjacques]

Looks good in general:

• PartitionScheme/PartitionSchemeDiscovery should be optional, there's a lot of the code that assumes otherwise. Such internal details should not leak in the exposed interface. Python lost the ability to not pass a schema.
• Drop the timestamp conversion stuff from this PR, make a seperate issue.

[fsaintjacques]

Use an optional parameter and merge the previous definitions. See discovery.cc about the behavior of this function, e.g. validation of explicit schema.

[bkietz]

alright

[bkietz]

Actually I'm not sure I agree with this one; default parameters in a virtual method pass the default case handling down to each implementation. Is there any behavior which should be supported for "Finish without explicit schema" other than "inspect a schema then finish with that"?

[fsaintjacques]

Then let's remove the virtual case for now :)

[fsaintjacques]

I think it's an anti-pattern to expect the user to call the Validate method so it doesn't blow up in his face. Anyhow, I'll tackle this in my next PR on DatasetDiscovery as discrepency will likely happen there more than in the DataSourceDiscovery.

[fsaintjacques]

The schema handling part of this body should be:

• If a schema is provided, validate it against Inspect result.
• If a schema is not provided, use Inspect result as the schema.

Think about other DataSource, e.g. OdbcData, how should it validate. The struct options in discovery is class specific, so maybe with functions:

// Validate infer against provided or use infered schema.
Finish(const std::shared_ptr<Schema>& schema = nullptr);
// Take as-is with no validation, might blow up at runtime.
FinishUnsafe(const std::shared_ptr<Schema>& schema);

[bkietz]

I think this is a valid concern but it's out of scope for this PR. Also, rather than separate Finish methods I think keeping the unsafe Finish(schema) overload and adding a Validate(schema) method would fit the interactive case better:

1. Inspect schema
2. tweak
3. validate
4. if failed, goto 2
5. finish

[bkietz]

@fsaintjacques there will always be a partition scheme; the default is DefaultPartitionScheme which is a noop and attaches scalar(true) to everything. I'll extract the timestamp conversion changes