Skip to content

Conversation

@aokolnychyi
Copy link
Contributor

This PR adds FlinkWriterFactory that extends BaseWriterFactory introduced in #2873.

@github-actions github-actions bot added the flink label Aug 3, 2021
@aokolnychyi
Copy link
Contributor Author

cc @stevenzwu @openinx

@aokolnychyi
Copy link
Contributor Author

I need a writer factory for Flink so that I can introduce new task writers.

builder.createWriterFunc((iSchema, typDesc) -> FlinkOrcWriter.buildWriter(dataFlinkType(), iSchema));
}

private RowType dataFlinkType() {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we move this logic to constructor?

Copy link
Contributor

Choose a reason for hiding this comment

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

or maybe move this part to the Builder#build() method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are optional and may not be set/needed unless we actually write data files.
Pure DELETEs will not need it, for example.

return this;
}

Builder dataFlinkType(RowType newDataFlinkType) {
Copy link
Contributor

@stevenzwu stevenzwu Aug 3, 2021

Choose a reason for hiding this comment

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

Is this an optional param? In the other comment, I saw we are converting Schema (above) to RowType if dataFlinkType is not set.

Maybe add Javadoc to mark all the *FlinkType methods/params as optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added docs to optional methods.

}

private RowType positionDeleteFlinkType() {
if (positionDeleteFlinkType == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

similar comment as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is similar to data files. The value is optional and may not be needed.

Copy link
Contributor

@stevenzwu stevenzwu left a comment

Choose a reason for hiding this comment

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

nice base test class

Copy link
Member

@openinx openinx left a comment

Choose a reason for hiding this comment

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

LGTM

@openinx openinx merged commit 0480c8c into apache:master Aug 4, 2021
@aokolnychyi
Copy link
Contributor Author

Thanks for reviewing, @openinx @stevenzwu!

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