[Fix][Connector-v2] Add DateMilliConvertor to Convert DateMilliVector into Default Timezone #8736

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

hailin0 merged 5 commits into apache:dev from TongchengOpenSource:arrow_localdatetime

Feb 18, 2025

Contributor

xiaochen-zhou commented Feb 14, 2025

Purpose of this pull request

Does this PR introduce any user-facing change?

The DateMilliVector type in Apache Arrow stores timestamps in UTC，Add DateMilliConvertor to Convert DateMilliVector into Default Timezone.

How was this patch tested?

exist tests

Check list

If any new Jar binary package adding in your PR, please add License Notice according
New License Guide
If necessary, please update the documentation to describe the new feature. https://github.com/apache/seatunnel/tree/dev/docs
If you are contributing the connector code, please check that the following files are updated:
1. Update plugin-mapping.properties and add new connector information in it
2. Update the pom file of seatunnel-dist
3. Add ci label in label-scope-conf
4. Add e2e testcase in seatunnel-e2e
5. Update connector plugin_config
Update the release-note.

xiaochen-zhou added 2 commits

February 15, 2025 04:03


          Fix ArrowToSeatunnelRowReader Date SqlType

a1965bf


          add DateMilliConvertor

e1d01fa

github-actions bot added the connectors-v2 label


          Fix code style

66ae659

xiaochen-zhou changed the title ~~[Fix][Zeta] Add DateMilliConvertor to Convert DateMilliVector into Default Timezone~~ [Fix][Connector-v2] Add DateMilliConvertor to Convert DateMilliVector into Default Timezone

Hisoka-X reviewed

View reviewed changes

Member

Hisoka-X left a comment

Could you add a test case to cover it?

Contributor Author

xiaochen-zhou commented Feb 17, 2025

Could you add a test case to cover it?

There is already this test case: ArrowToSeatunnelRowReaderTest#testSeatunnelRow(). If this conversion is not added, it may cause this test case to fail, for example, before 8 a.m.

Contributor Author

xiaochen-zhou commented Feb 17, 2025

Could you add a test case to cover it?

Store Timestamp：1739564483396

Member

Hisoka-X commented Feb 17, 2025

Could you add a test case to cover it?

There is already this test case: ArrowToSeatunnelRowReaderTest#testSeatunnelRow(). If this conversion is not added, it may cause this test case to fail, for example, before 8 a.m.

I think we can use a fixed datetime (not use system time now()) to reproduce this bug.

Member

Hisoka-X commented Feb 17, 2025

If this conversion is not added, it may cause this test case to fail, for example, before 8 a.m.

Any test case failed link?

Contributor Author

xiaochen-zhou commented Feb 17, 2025

If this conversion is not added, it may cause this test case to fail, for example, before 8 a.m.

Any test case failed link?

No, I discovered this during local debugging. This test case is likely to reproduce the issue if run before 8 a.m.

Contributor Author

xiaochen-zhou commented Feb 17, 2025

Could you add a test case to cover it?

Store Timestamp：1739564483396

Member

Hisoka-X commented Feb 17, 2025

If this conversion is not added, it may cause this test case to fail, for example, before 8 a.m.

Any test case failed link?

No, I discovered this during local debugging. This test case is likely to reproduce the issue if run before 8 a.m.

OK, please add test case.

Contributor Author

xiaochen-zhou commented Feb 17, 2025

Could you add a test case to cover it?

There is already this test case: ArrowToSeatunnelRowReaderTest#testSeatunnelRow(). If this conversion is not added, it may cause this test case to fail, for example, before 8 a.m.

I think we can use a fixed datetime (not use system time now()) to reproduce this bug.

Ok, I will refer to this to add the test case. @Hisoka-X


          update ArrowToSeatunnelRowReaderTest

360262b

hawk9821 reviewed

View reviewed changes

...apache/seatunnel/connectors/seatunnel/common/source/arrow/ArrowToSeatunnelRowReaderTest.java

    
                   * on windows The test case uses TimeStampMicroVector to test the timestamp, thus truncating the

                   * timestamp accuracy to ChronoUnit.MILLIS

                   */

                  private static final LocalDateTime localDateTime =

Contributor

hawk9821 Feb 17, 2025

Suggested change

      
                private static final LocalDateTime localDateTime =
          
                private static final LocalDateTime localDateTime =
          
                        LocalDateTime.parse(
          
                                "2025-02-15 02:21:23", DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss"));

...apache/seatunnel/connectors/seatunnel/common/source/arrow/ArrowToSeatunnelRowReaderTest.java

    
                          // map

                      }

                      // allocate storage

                      vectors.forEach(FieldVector::allocateNew);

Contributor

hawk9821 Feb 17, 2025

Suggested change

      
                    vectors.forEach(FieldVector::allocateNew);
          
                    vectors.forEach(FieldVector::allocateNew);
          
                    long epochMilli = localDateTime.atZone(zoneId).toInstant().toEpochMilli();

...apache/seatunnel/connectors/seatunnel/common/source/arrow/ArrowToSeatunnelRowReaderTest.java Outdated

    
                                                  .setSafe(i, (stringValue).getBytes(StandardCharsets.UTF_8));

                                      } else if (vector instanceof TimeStampMicroVector) {

                                          ((TimeStampMicroVector) vector).setSafe(i, epochMilli * 1000);

                                          ((TimeStampMicroVector) vector).setSafe(i, testEpochMilli);

Contributor

hawk9821 Feb 17, 2025

Suggested change

      
                                        ((TimeStampMicroVector) vector).setSafe(i, testEpochMilli);
          
                                        ((TimeStampMicroVector) vector).setSafe(i, epochMilli * 1000);

...apache/seatunnel/connectors/seatunnel/common/source/arrow/ArrowToSeatunnelRowReaderTest.java Outdated

    
                                                  .setSafe(i, (stringValue).getBytes(StandardCharsets.UTF_8));

                                      } else if (vector instanceof TimeStampMilliTZVector) {

                                          ((TimeStampMilliTZVector) vector).setSafe(i, epochMilli);

                                          ((TimeStampMilliTZVector) vector).setSafe(i, testEpochMilli);

Contributor

hawk9821 Feb 17, 2025

Suggested change

      
                                        ((TimeStampMilliTZVector) vector).setSafe(i, testEpochMilli);
          
                                        ((TimeStampMilliTZVector) vector).setSafe(i, epochMilli);

...apache/seatunnel/connectors/seatunnel/common/source/arrow/ArrowToSeatunnelRowReaderTest.java Outdated

    
                                          ((TimeStampMilliTZVector) vector).setSafe(i, testEpochMilli);

                                      } else if (vector instanceof TimeMicroVector) {

                                          ((TimeMicroVector) vector).setSafe(i, epochMilli);

                                          ((TimeMicroVector) vector).setSafe(i, testEpochMilli);

Contributor

hawk9821 Feb 17, 2025

ditto

...apache/seatunnel/connectors/seatunnel/common/source/arrow/ArrowToSeatunnelRowReaderTest.java Outdated

    
                                          ((TimeMicroVector) vector).setSafe(i, testEpochMilli);

                                      } else if (vector instanceof DateMilliVector) {

                                          ((DateMilliVector) vector).setSafe(i, epochMilli);

                                          ((DateMilliVector) vector).setSafe(i, testEpochMilli);

Contributor

hawk9821 Feb 17, 2025

ditto

...apache/seatunnel/connectors/seatunnel/common/source/arrow/ArrowToSeatunnelRowReaderTest.java Outdated

    
                                                  List<LocalDateTime> dateTimeList = new ArrayList<>();

                                                  for (int j = 0; j < 5; j++) {

                                                      writer.writeTimeStampMilliTZ(epochMilli);

                                                      writer.writeTimeStampMilliTZ(testEpochMilli);

Contributor

hawk9821 Feb 17, 2025

ditto


          update ArrowToSeatunnelRowReaderTest

cde927e

Contributor Author

xiaochen-zhou commented Feb 18, 2025

@Hisoka-X @hawk9821 done.

Hisoka-X requested a review from hawk9821

February 18, 2025 06:09

hawk9821 approved these changes

View reviewed changes

Contributor

hawk9821 left a comment

LGTM. Thanks @xiaochen-zhou

github-actions bot added the reviewed label

Hisoka-X approved these changes

View reviewed changes

github-actions bot added the approved label

hailin0 approved these changes

View reviewed changes

hailin0 merged commit 7b8298a into apache:dev

3 checks passed

xiaochen-zhou deleted the arrow_localdatetime branch

February 22, 2025 16:53

zax1314157 pushed a commit to zax1314157/seatunnel that referenced this pull request


          [Fix][Connector-v2] Add DateMilliConvertor to Convert DateMilliVector…

b410eb2

… into Default Timezone (apache#8736)

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

Labels

approved connectors-v2 reviewed