Apply timezone to Timestamp when using bulkcopy for batch insert

[tkyc]

Correctly applies the timezone to Timestamps when inserted using batch insert with bulkcopy.

Fixes #2271

[PriyadharshiniP]

Hi @tkyc

I appreciate you making the changes.

I observed that the fix had the following problems:

1. Could you kindly have a look at the applyTimezoneTo method, which appears to use
   Date date = new Date(cal.getTimeInMillis()); rather than Date date = new Date(ts.getTime()); ?
   If you have a time value other than today's time in your unit test, BatchExecutionTest for testValidTimezoneForTimestampBatchInsertWithBulkCopy, it will begin to break. The unit test would have seemed to be successful because the timestamp and calendar will often have the same current time value. However, this unit test would begin to fail if you had a different timestamp value.
   For example, the unit test failed when I modified the timestamp milliseconds from
   long ms = Timestamp.valueOf(todayString).getTime(); to long ms = 1433338533461L;. that leads to the failure.

2. In the event that the batch insert for bulk copy API falls back to the original batch insert, this fix does not seem to function. An additional conversion occurs when the fallback to no bulk copy API is used, leading to a double conversion for GMT.
   To replicate this issue, Use the existing BulkCopyResultSetCursorTest for testMultiplePreparedStatementAndResultSet unit test method alone. Connection string should have "useBulkCopyForBatchInsert=true;sendTemporalDataTypesAsStringForBulkCopy=false;".
   As bulk copy does not allow IDENTITY columns, using bulk copy would fail and result in inaccurate database values with fallback.
   

May I ask if the changes need to be done at SQLServerBulkCopy ?

[tkyc]

@PriyadharshiniP Thanks for the feedback. I'll do more investigating and I'll look into correcting the implementation.

May I ask if the changes need to be done at SQLServerBulkCopy ?

That was my initial thought also, but when I looked over SQLServerBulkCopy it doesn't look like I'm able to apply the timezone in the TDS request for bulkcopy.

For example, I sort of expected code like this in SQLServerBulkCopy, which is part of the normal and regular batch insert code path. The code below takes into consideration the the timezone by sending the timezone info in the TDS request. So the timezone is applied during insertion by the server:

                                    tdsWriter.writeEncryptedRPCDateTime2(name,
                                            timestampNormalizedCalendar(calendar, javaType, conn.baseYear()),
                                            subSecondNanos, (valueLength), isOutParam, statement);

But I'm guessing because we're doing a bulkcopy, it has to send the timestamp value as is. And so, either the user or driver needs to apply the timezone conversion beforehand.

[PriyadharshiniP]

1. BatchExecutionTest
   Hi @tkyc

Thanks for fixing the issue.

1. Additionally, it appears that not all dates—particularly those with daylight savings time—cannot be converted successfully using the applyTimezoneTo method . In your BatchExecutionTest unit test case , if you change the ms to long ms = 1696127400000L; that equates to :

GMT: Sunday, October 1, 2023 2:30:00 AM
My local time zone: Sunday, October 1, 2023 1:30:00 PM [GMT+11:00]

This time "GMT: Sunday, October 1, 2023 2:30:00 AM" doesn't exist in my local time zone. The non bulk version correctly inserts the date as 2023-10-01 02:30:00.000 but the bulk version incorrectly inserts as 2023-10-01 03:30:00.000 and the BatchExecutionTest will fail if we use ts0 = rs.getTimestamp(1, gmtCal);. and ts1 = rs.getTimestamp(1, gmtCal);
I think this is because of using Timestamp.valueOf or any timestamp related methods that will use only local time for conversions.

2. As the standard batch insert without bulk copy seems to work in all cases, I started to look re-use the tdsWriter.writeEncryptedRPCDateTime2 in SQLServerBulkCopy itself, however while attempting to fix it, I encounter certain problems.
   To store the parameters needed for the writeEncryptedRPCDateTime2 method, I made a new class. Next, update the SQLServerBulkBatchInsertRecord class with those values. Fill in the datetime2 field in SQLServerBulkCopy in accordance with the screenshots below :
   
   
   However, the following error appears: java.sql.BatchUpdateException: While reading current row from host, a premature end-of-message was encountered--an incoming data stream was interrupted when the server expected to see more data. The host program may have terminated. Ensure that you are using a supported client application programming interface (API).
   Kindly inform me if you can assist with this.

Could you see if we can accomplish something along these lines?

[tkyc]

@PriyadharshiniP Thanks for collaborating.

As the standard batch insert without bulk copy seems to work in all cases, I started to look re-use the tdsWriter.writeEncryptedRPCDateTime2 in SQLServerBulkCopy

Yeah, I struggled with getting that working as well when I initially looked at the code. In SQLServerBulkCopy, it seems the only thing written to the TDS request is column metadata info and data type info and it doesn't look like any input values are written eg. the timestamp in our case for batch insert with bulkcopy. When I traced the batch insert bulkcopy call path:

writeToServer -> sendBulkLoadBCP -> ... -> doInsertBulk -> sendBulkCopyCommand

In the above call path, I expected to end up somewhere where the timestamp value is written to the TDS request (something/somewhere where the logic writeEncryptedRPCDateTime2 can be used) but I don't. I'll do some more debugging, maybe I missed some things initially.

Additionally, it appears that not all dates—particularly those with daylight savings time—cannot be converted successfully using the applyTimezoneTo method

Thanks for the help on testing. I'll see what I can do here...

[tkyc]

@PriyadharshiniP What's your exact local time zone? I've switched my PC's time zone setting to various different time zones that doesn't have day light savings and wasn't able to repro the issue with 1696127400000L. I double checked the local time zone used in my JDK and it matches the PC time zone set when I was experimenting.

Maybe I need to do something like below explicitly to repro your issue?

TimeZone tz = TimeZone.getTimeZone("your-time-zone");
TimeZone.setDefault(tz);

[PriyadharshiniP]

My local timezone is Australia/Sydney

…

On Wed, 17 Jan 2024, 8:11 am Terry Chow, ***@***.***> wrote: @PriyadharshiniP <https://github.com/PriyadharshiniP> What's your exact local time zone? I've switched my PC's time zone setting to various different time zones that doesn't have day light savings and wasn't able to repro the issue. I double checked the local time zone used in my JDK and it matches my PC time zone. — Reply to this email directly, view it on GitHub <#2291 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABXI4WXKWWK5KQJQM6BSDXLYO3UHDAVCNFSM6AAAAABBVM2HUSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJUGUYTQOBUGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[PriyadharshiniP]

@PriyadharshiniP What's your exact local time zone? I've switched my PC's time zone setting to various different time zones that doesn't have day light savings and wasn't able to repro the issue with 1696127400000L. I double checked the local time zone used in my JDK and it matches the PC time zone set when I was experimenting.

Maybe I need to do something like below explicitly to repro your issue?

TimeZone tz = TimeZone.getTimeZone("your-time-zone");
TimeZone.setDefault(tz);

I guess you could do something like this in your unit test :System.setProperty("user.timezone","Australia/Sydney");

[tkyc]

@PriyadharshiniP I've just pushed a new fix implementation. I did some testing of it and it seem to have addressed all your concerns so far. Let me know if there's any additional problems. Thanks again for the collab.

[PriyadharshiniP]

@PriyadharshiniP I've just pushed a new fix implementation. I did some testing of it and it seem to have addressed all your concerns so far. Let me know if there's any additional problems. Thanks again for the collab.

Hi @tkyc
Thanks for fixing it so quickly .
I checked all dates this year in my local time zone, and it seems that there will still be problems with the bulk copy inserting the date in my local environment around spring forward dates. The time stamp millisecond values is 1712415600000L and 1712417400000L, to be more precise.
Noted the use of java.sql.Timestamp.valueOf(colValue.toString()) in SQLServerBulkCopy, this seems to be the cause of this issue as it seems to alter the timestamp milliseconds value. Using the (Timestamp)colValue seems to work for all dates. Could you please check and fix this as well?

[tkyc]

@PriyadharshiniP Oops, my mistake. Calling valueOf will cause a conversion. Fixed.

[PriyadharshiniP]

@PriyadharshiniP Oops, my mistake. Calling valueOf will cause a conversion. Fixed.

Thanks @tkyc :
It seems to be fine for my tests.
Please let me know when the release would be available ?

Regards

[tkyc]

@PriyadharshiniP this will make it in the release that's on the 31st of this month.