[BEAM-6675] Generate JDBC statement and preparedStatementSetter automatically when schema is available #8962

JawadHyder · 2019-06-28T12:01:42Z

Added functionality to automatically generate JDBC insert statement and preparedStatementSetter when there is schema attached to the PCollection. This functionality is triggered when the user has not provided withStatement and withPreparedStatementSetter during JdbcIO.write() operation.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	Gearpump	Samza
Go	---	---	---	---
Java
Python	---		---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

JawadHyder · 2019-06-28T12:37:43Z

retest this please

JawadHyder · 2019-06-28T13:06:52Z

R: @reuvenlax
R: @jbonofre

JawadHyder · 2019-06-28T18:18:36Z

Run Java PreCommit

JawadHyder · 2019-06-30T10:50:05Z

Run JavaPortabilityApi PreCommit

…atically when schema is available

salmanVD · 2019-07-01T14:10:18Z

Run Java PreCommit

amaliujia · 2019-07-08T18:10:27Z

sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcUtil.java

+
+  /** PreparedStatementSetCaller for Schema Field types. * */
+  private static Map<Schema.TypeName, JdbcIO.PreparedStatementSetCaller> typeNamePsSetCallerMap =
+      new EnumMap<Schema.TypeName, JdbcIO.PreparedStatementSetCaller>(


Seems missing handling some types in Schema?

O NVM. There is a separate section to handle other types (array, logical types, etc.)

Actually BYTES is not handled?

Bytes is handled as well https://github.com/JawadHyder/beam/blob/9bb7280a65a4ea506f36d8597aaee3500633ef5a/sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcUtil.java#L96

amaliujia · 2019-07-08T18:13:12Z

sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcUtil.java

+            case DATE:
+              return (element, ps, i, fieldWithIndex) -> {
+                ps.setDate(
+                    i + 1, new Date(element.getDateTime(fieldWithIndex.getIndex()).getMillis()));


Question: is it correct to set epoch millis for Date and Time type?

The constructor for Date & Time takes a long value so I don't see any issue setting epoch for date & time. Do you have any other thoughts on this?

I guess here we should obey java.sql.Date and java.sql.Time's specs.

https://docs.oracle.com/javase/7/docs/api/java/sql/Date.html
https://docs.oracle.com/javase/8/docs/api/index.html?java/sql/Time.html

As per my understanding we should use these constructors instead of epoch
Time - Time(int hour, int minute, int second)
Date - Date(int year, int month, int day)
but both are deprecated as per docs, are you referring to the same thing @amaliujia ?

Something similar, for example:

The date components should be set to the "zero epoch" value of January 1, 1970 and should not be accessed.

I am not sure if current construction for TIME is doing this since the millis is used directly.

updated for both date and time objects
done

amaliujia · 2019-07-08T18:15:08Z

sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcUtil.java

+              return (element, ps, i, fieldWithIndex) -> {
+                ps.setTimestamp(
+                    i + 1,
+                    new Timestamp(element.getDateTime(fieldWithIndex.getIndex()).getMillis()));


As this is a timezoned type, is time zone handled here?

@amaliujia there is a method in PreparedStatement.java setTimestamp which takes Calendar instance as third argument. Can we use this to set UTC timezone? of any other suggestion you have

I am thinking element.getDateTime will return a Joda Datetime, which has timezone information already?

timestamp is the moment in time I don't think timezone is needed here because when user is going to read data from DB (timestamp field) then it needs to be converted at that point.
Please check this link https://stackoverflow.com/questions/15994450/joda-datetime-to-timestamp-conversion/15994657#15994657
can you share something related to your point? may be I am missing something here

There are two TIMESTAMP: TIMESTAMP without TIMEZONE(sometimes I saw it's called DATETIME), TIMESTAMP with TIMEZONE.

In this implementation, it's processing a TIMESTAMP with TIMEZONE type, which definitely has a timezone information that matters. I think here are two concepts matter (which is also covered by your link): an absolute point in time and an offset. Offset (specified by timezone) should not be lost for TIMESTAMP with TIMEZONE

I find this SO question is useful: https://stackoverflow.com/questions/14070572/is-java-sql-timestamp-timezone-specific

used setTimestamp with calendar instance for timezone
done

amaliujia · 2019-07-08T18:19:14Z

sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcUtil.java

+
+  private static void validateLogicalTypeLength(Schema.Field field, Integer length) {
+    if (field.getType().getTypeName().isLogicalType()
+        && length >= Integer.parseInt(field.getType().getLogicalType().getArgument())) {


I think logical type argument is not always set. If the argument is not set as an Integer, maybe we can further check base type?

@amaliujia the case is if the string length of input field exceeds from the table max limit then exception should be raised, base type will still be same even if the length differs.

If so, could we check input field to see if it is bytes or string?

Input fields are checked in the switch case then called createStringCaller() for string for bytes it's createBytesCaller()
https://github.com/JawadHyder/beam/blob/9bb7280a65a4ea506f36d8597aaee3500633ef5a/sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcUtil.java#L155
If only type check is enough then wouldn't it truncate the string/bytes if it exceeds the max limit from the table?

O I see. Then that's good enough. I wasn't sure how this function is called. E.g. If this function is called to verify other types like Date, Time, etc.

No these methods are called for string and bytes due to variable length. So one thing, if the argument is not set should we throw exception (the field details like argument is fetched from table schema so I don't think it can be null) or let it proceed with the insertion?

Argument is an optional parameter. So I think if it is not set, we can proceed to insertion.

So there are 3 cases:

If argument is not set - bypass the check and proceed with insertion (can we use a warning message here as data might truncate due to length mismatch?)

If argument is set and it is not integer - do same as above

If argument is set and it is integer - throw exception if length mismatch

Let's finalize these cases or any other case you think might possible or we can raise separate jira for this and remove checks from here?

Sounds good to all of these. It would be backward-compatible to allow bypassing checking in 1 and 2 such that existing types won't break if setting an argument.

snaeemk · 2019-07-09T10:18:45Z

Run Java PreCommit

snaeemk · 2019-07-12T06:42:12Z

@amaliujia can you please review the comments again?

amaliujia · 2019-07-12T17:04:00Z

Thanks @sehrish-vd and @JawadHyder for working on this PR! Responded to comments!

…added method for date and time only

snaeemk · 2019-07-15T15:06:31Z

Run Java PreCommit

amaliujia · 2019-07-16T04:25:31Z

LGTM

lgajowy · 2019-07-16T09:19:07Z

Run Java JdbcIO Performance Test

amaliujia · 2019-07-16T19:44:16Z

Thanks @lgajowy!

Jawad Hyder added 4 commits July 1, 2019 13:25

[BEAM-6675] Generate JDBC statement and preparedStatementSetter autom…

0d1edb8

…atically when schema is available

gradlew spotlessApply

dde62ec

Fix CheckStyle warnings

a7855cf

Fix test case

e7d4160

JawadHyder force-pushed the BEAM-6675 branch from cc161bd to e7d4160 Compare July 1, 2019 08:30

Jawad Hyder and others added 2 commits July 1, 2019 13:31

Fix import

9665c6c

Leak connection fix

9bb7280

apilloud requested review from jbonofre and reuvenlax July 8, 2019 17:56

amaliujia reviewed Jul 8, 2019

View reviewed changes

Sehrish Naeem added 2 commits July 9, 2019 14:35

BEAM-6675 | set UTC timezone as default for TIMESTAMP_WITH_TIMEZONE

26cf151

BEAM-6675 | set UTC timezone as default for TIMESTAMP_WITH_TIMEZONE

da6c551

BEAM-6675 | reverted UTC timezone with calendar instance

64bd4c9

Sehrish Naeem added 2 commits July 15, 2019 19:01

BEAM-6675 | timestamp with timezone handled in preparedStatement and …

6500323

…added method for date and time only

BEAM-6675 | run spotlessApply on jdbc module

0994514

BEAM-6675 | run spotlessApply

7cb6689

lgajowy approved these changes Jul 16, 2019

View reviewed changes

lgajowy merged commit 41478d0 into apache:master Jul 16, 2019

[BEAM-6675] Generate JDBC statement and preparedStatementSetter automatically when schema is available #8962

[BEAM-6675] Generate JDBC statement and preparedStatementSetter automatically when schema is available #8962

Conversation

JawadHyder commented Jun 28, 2019 • edited Loading

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

JawadHyder commented Jun 28, 2019

JawadHyder commented Jun 28, 2019

JawadHyder commented Jun 28, 2019

JawadHyder commented Jun 30, 2019

salmanVD commented Jul 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaliujia Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaliujia Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snaeemk Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaliujia Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snaeemk Jul 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snaeemk commented Jul 9, 2019

snaeemk commented Jul 12, 2019

amaliujia commented Jul 12, 2019

snaeemk commented Jul 15, 2019

amaliujia commented Jul 16, 2019

lgajowy commented Jul 16, 2019

amaliujia commented Jul 16, 2019

JawadHyder commented Jun 28, 2019 •

edited

Loading

amaliujia Jul 8, 2019 •

edited

Loading

amaliujia Jul 12, 2019 •

edited

Loading

snaeemk Jul 9, 2019 •

edited

Loading

amaliujia Jul 12, 2019 •

edited

Loading

snaeemk Jul 10, 2019 •

edited

Loading