Skip to content

[BEAM-2079] Support TextIO as SQL source/sink#2942

Closed
xumingming wants to merge 4 commits intoapache:DSL_SQLfrom
xumingming:BEAM-2079
Closed

[BEAM-2079] Support TextIO as SQL source/sink#2942
xumingming wants to merge 4 commits intoapache:DSL_SQLfrom
xumingming:BEAM-2079

Conversation

@xumingming
Copy link
Contributor

Support TextIO as SQL source/sink

@xumingming xumingming changed the base branch from master to DSL_SQL May 7, 2017 07:39
@xumingming
Copy link
Contributor Author

Retest this please.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling bc9f9f3 on xumingming:BEAM-2079 into ** on apache:DSL_SQL**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling bc9f9f3 on xumingming:BEAM-2079 into ** on apache:DSL_SQL**.

@xumingming
Copy link
Contributor Author

Retest this please

@xumingming
Copy link
Contributor Author

R: @xumingmin

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling bc9f9f3 on xumingming:BEAM-2079 into ** on apache:DSL_SQL**.

CSVParser parser = csvFormat.parse(reader);
rawRecord = parser.getRecords().get(0);
} catch (IOException e) {
LOG.error("error record: " + str, e);
Copy link

Choose a reason for hiding this comment

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

throw exception here to report error? same for L76

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that some error records should not fail the whole job. But lets be strict for now.

row.addField(idx, raw);
break;
default:
row.addField(idx, raw);
Copy link

Choose a reason for hiding this comment

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

throw BeamSqlUnsupportedException instead, row.addField(idx, raw); always fail here.

}
printer.println();
} catch (IOException e) {
LOG.error("invalid record: " + row, e);
Copy link

Choose a reason for hiding this comment

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

throw exception

public void processElement(ProcessContext ctx) {
String str = ctx.element();

StringReader reader = new StringReader(str);
Copy link

Choose a reason for hiding this comment

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

need to close StringReader at the end of 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.

missed that :(

@mingmxu
Copy link

mingmxu commented May 9, 2017

Let's use TestPipeline described here to update the test cases for eamTextCSVTableIOWriter.java, BeamTextCSVTableIOReader.java.

@xumingming
Copy link
Contributor Author

Test updated.

@xumingming
Copy link
Contributor Author

R: @jbonofre

@mingmxu
Copy link

mingmxu commented May 9, 2017

LGTM
@jbonofre could you also take a look?

@jbonofre
Copy link
Member

Let me take a look.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 99ce0aa on xumingming:BEAM-2079 into ** on apache:DSL_SQL**.

Copy link
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

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

LGTM, I'm merging.

asfgit pushed a commit that referenced this pull request May 10, 2017
@jbonofre
Copy link
Member

I merged on DSL_SQL branch. You can close this PR. Thanks !

@xumingming xumingming closed this May 10, 2017
@xumingming xumingming deleted the BEAM-2079 branch May 10, 2017 05:26
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c35da2c on xumingming:BEAM-2079 into ** on apache:DSL_SQL**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c35da2c on xumingming:BEAM-2079 into ** on apache:DSL_SQL**.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants