Skip to content

API: Extend FileIO in optional interfaces#5576

Merged
rdblue merged 2 commits intoapache:masterfrom
aokolnychyi:extend-io-bulk
Aug 21, 2022
Merged

API: Extend FileIO in optional interfaces#5576
rdblue merged 2 commits intoapache:masterfrom
aokolnychyi:extend-io-bulk

Conversation

@aokolnychyi
Copy link
Contributor

This PR extends FileIO in its optional interfaces. This way, we we can access all methods available in FileIO if we have a reference to the optional interface. Otherwise, we have to cast back.

@github-actions github-actions bot added the API label Aug 18, 2022
@aokolnychyi
Copy link
Contributor Author

@danielcweeks @rdblue @flyrain @szehon-ho @stevenzwu @jackye1995, could you take a look?

* libraries.
*/
public interface CredentialSupplier {
public interface CredentialSupplier extends FileIO {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All these interfaces assume they are mixed-in by FileIO instances (see docs).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not quite correct. CredentialSupplier may extend other interfaces and not just FileIO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I misinterpreted the doc.

@rdblue
Copy link
Contributor

rdblue commented Aug 19, 2022

I don't think we should do this for CredentialSupplier, but the other two look fine.

@stevenzwu
Copy link
Contributor

@aokolnychyi this doesn't seem to be a problem in current code. I assume you ran into this need of casting in some new code you are working on?

I found the interface name of SupportsPrefixOperations less intuitive for an interface extending from FileIO. Supports just meant some extra capability. can't connect Supports... as an extended interface of FileIO.

Another downside is that S3FileIO would need to implement both SupportsBulkOperations and SupportsPrefixOperations with large overlap of base FileIO interface. not a problem for compiler. just thought a little odd.

@aokolnychyi
Copy link
Contributor Author

@stevenzwu, this becomes an issue when we need both the optional functionality as well the core FileIO methods. I don't think these SupportsXXX can be used without FileIO as they are tightly coupled with it. It's the same pattern that Spark, for example, uses in its public APIs. These SupportsXXX simply add optional functionality to FileIO.

@stevenzwu
Copy link
Contributor

stevenzwu commented Aug 21, 2022

agree that SupportsXXX can't be used without FileIO, as SupportsXXX declares additional capability for FileIO.

public class S3FileIO
    implements FileIO, SupportsBulkOperations, SupportsPrefixOperations

I am trying to understand the concrete issue (type cast problem) that this tries to help with. Are we trying to type cast a FileIO object to SupportsXXX interface to check the capability? Can we use instanceOf to check the capability?

if we want to access FileIO methods after type cast say SupportsBulkOperations, it won't help if we also need to access the methods from SupportsPrefixOperations. We will still need to type cast to two different SupportsXXX interfaces.

@rdblue rdblue merged commit f06c4f7 into apache:master Aug 21, 2022
@rdblue
Copy link
Contributor

rdblue commented Aug 21, 2022

Thanks, @aokolnychyi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants